Changes between Version 39 and Version 40 of Guidelines/MaintenanceGuidelines

Timestamp:

Feb 3, 2010, 8:58:47 PM (13 years ago)

Author:

phorgan1

Comment:

--

Guidelines/MaintenanceGuidelines

@@ -903 +903 @@
 More information needed here!
 
-== Individual Warnings ==
+=== Individual Warnings ===
 '''comparison between signed and unsigned integer expressions'''
 
@@ -1036 +1036 @@
         }}}
 
-    and you had a keyword list to pass it like this:
+        and you had a keyword list to pass it that looked like this:
 
         {{{
@@ -1044 +1044 @@
         }}}
 
-        you would get the warning on the kwlist assignment, but when you change
+        you would get the warning on the kwlist assignment, because you're 
+        assigning a const char * to char *, but when you correctly change
         it to:
 
@@ -1051 +1052 @@
         }}}
 
-        you'll get:
+        to get rid of the waring you'll get a warning on the call to 
+        PyArg_ParseTupleAndKeywords:
 
             error: invalid conversion from ‘const char**’ to ‘char**’
 
-        unless you change the call to:
+        because the library function was declared as taking a char **
+        for the fourth argument.  Your only recourse is to cast away
+        the constness with a const_cast:
 
         {{{
@@ -1062 +1066 @@
         }}}
 
-        annoyingly having to incorrectly cast away the constness of kwlist.
-        Now the code is arguably correct and it wasn't before.
+        Annoying, yet more correct than before.
+
+'''dereferencing type-punned pointer will break strict-aliasing rules'''
+    this warning is only active when strict-aliasing optimization is active
+
+    '''-Wstrict-aliasing''' - equivalent to -Wstring-aliasing=3 (included in -Wall)[[BR]]
+    '''-Wstrict-aliasing=N''' where N is in {1,2,3} - Higher N implies less false positives, at the expense of more work.  Currently there are 3 levels.[[BR]]
+        Level 1: Most agressive, quick, very few false negatives, but many false positives.  Might use if higher levels don't warn, but turning on -fstrict-aliasing breaks the code.  Warns about pointer conversions even if pointer not dereferenced.[[BR]]
+        Level 2: Agressive, quick, not too precise.  Less false positives than Level 1, but more false negatives.  Only warns if pointer is dereferenced.[[BR]]
+        Level 3: Very few false positives and very few false negatives--the default.[[BR]]
+    '''-fstrict-aliasing''' - also turned on by -O2, -O3 and -Os.  Tells the compiler that it's ok to do a certain class of optimization based on the type of expressions.  In particular you're promising by using this flag that an object of one type won't reside at the same address as an object of an incompatible type.
+    '''-fno-strict-aliasing''' - turns off this optimization.  If this changes the behavior of your code, you have a problem in your code.
+
+    This is trying to let you know that you are asking the compiler to do undefined behavior and it may not do what you think it will do.  As the optimization level increases, the liklihood that you won't like what it does will increase.  I show a simple example later that surprisingly generates the wrong result when optimization at any level is turned on.  Ignore this warning at your own peril.  You are unlikely to care for the undefined behavior that results.
+{{{
+From the C++ Standard, section 3.10 Lvalues and rvalues
+      15   If a program attempts to access the stored value of an object through an lvalue of other than
+          one of the following types the behavior is undefined
+        — the dynamic type of the object,
+        — a cv-qualified version of the dynamic type of the object,
+        — a type similar (as defined in 4.4) to the dynamic type of the object,
+        — a type that is the signed or unsigned type corresponding to the dynamic type of the object,
+        — a type that is the signed or unsigned type corresponding to a cv-qualified version of the dynamic type of the object,
+        — an aggregate or union type that includes one of the aforementioned types among its member
+          (including, recursively, a member of a subaggregate or contained union),
+        — a type that is a (possibly cv-qualified) base class type of the dynamic type of the object,
+        — a char or unsigned char type.
+}}}
+The following program generates 6 warnings about breaking strict-aliasing rules, and many would dismiss them.  The correct output of the program is:
+
+00000020
+00200000
+
+but when optimization is turned on it's:
+
+00000020
+00000020
+
+THAT's what the warning is trying to tell you, that the optimizer is going to do things that you don't like.  In this case seeing that acopy is set to a and never touched again, strict aliasing lets it optimize by just returning the original value of a at the end.
+{{{
+uint32_t
+swaphalves(uint32_t a)
+{
+    uint32_t acopy=a;
+    uint16_t *ptr=(uint16_t*)&acopy;// can't use static_cast<>, not legal.
+                                    // you should be warned by that.
+    uint16_t tmp=ptr[0];
+    ptr[0]=ptr[1];
+    ptr[1]=tmp;
+    return acopy;
+}
+
+int main()
+{
+    uint32_t a;
+    a=32;
+    cout << hex << setfill('0') << setw(8) << a << endl;
+    a=swaphalves(a);
+    cout << setw(8) << a << endl;
+}
+}}}
+Here's the (annotated) x86 assembler generated by gcc 4.4.1 for swaphalves, let's see what went wrong:
+{{{
+_Z10swaphalvesj:
+    pushl   %ebp
+    movl    %esp, %ebp
+    subl    $16, %esp
+    movl    8(%ebp), %eax   # get a in %eax
+    movl    %eax, -8(%ebp)  # and store in in acopy
+    leal    -8(%ebp), %eax  # now get eax pointing at acopy (ptr=&acopy)
+    movl    %eax, -12(%ebp) # save that ptr at -12(%ebp)
+    movl    -12(%ebp), %eax # get the ptr back in %eax
+    movzwl  (%eax), %eax    # get 16 bits from ptr[0] in eax
+    movw    %ax, -2(%ebp)   # store the 16 bits into tmp
+    movl    -12(%ebp), %eax # get the ptr back in eax
+    addl    $2, %eax        # bump up by two to get to ptr[1]
+    movzwl  (%eax), %edx    # get that 16 bits into %edx
+    movl    -12(%ebp), %eax # get ptr into eax
+    movw    %dx, (%eax)     # store the 16 bits into ptr[1]
+    movl    -12(%ebp), %eax # get the ptr again
+    leal    2(%eax), %edx   # get the address of ptr[1] into edx
+    movzwl  -2(%ebp), %eax  # get tmp into eax
+    movw    %ax, (%edx)     # store into ptr[1]
+    movl    -8(%ebp), %eax  # forget all that, return original a.
+    leave
+    ret
+}}}
+Scary, isn't it?  Of course you could use -fno-strict-aliasing to get the right output, but the generated code won't be as good.  A better way to accomplish the same thing without the warning's or the incorrect output is to define swaphalves like this:
+
+{{{
+uint32_t
+swaphalves(uint32_t a)
+{
+    union swapem{
+        uint32_t a;
+        uint16_t b[2];
+    };
+    swapem s={a};
+    uint16_t tmp;
+    tmp=s.b[0];
+    s.b[0]=s.b[1];
+    s.b[1]=tmp;
+    return s.a;
+}
+}}}
+The C++ compiler knows that members of a union alias, and this helps the compiler generate MUCH better code:
+{{{
+_Z10swaphalvesj:
+    pushl   %ebp                        # save the original value of ebp
+    movl    %esp, %ebp          # point ebp at the stack frame
+    movl    8(%ebp), %eax       # get a in eax
+    popl    %ebp                        # get the original ebp value back
+    roll    $16, %eax           # swap the two halves of a and return it
+    ret
+}}}
+
+    
+
+
+
+
 
  |? || class does not have a  virtual destructor||Suppress or provide a virtual destructor.||??||