kicad-developers team mailing list archive

[Michael Narigon <mnarigon@xxxxxxxxx>]

On Jul 30, 2014, at 2:19 PM, Dick Hollenbeck <dick@xxxxxxxxxxx> wrote:

> On 07/30/2014 02:51 PM, Michael Narigon wrote:
>> All, Here are four patches to correct some warnings I am seeing while compiling with
>> the OS X compiler in C++11 mode. I think they have general applicability for all
>> targets. I checked that the patches will apply against 5037 and that the 5037 programs
>> run (on Ubuntu Linux 14.04).
>> 
>> The first patch fixes a typo in the header guard for
>> pcb_calculator/datafile_read_write.h. The typo would cause the guard to fail.
>> 
>> 
>> 
>> 
>> 
>> The second patch fixes some unsigned comparisons in two files with similar logic
>> (cvpcb/class_footprints_listbox.cpp and cvpcb/class_library_listbox.cpp.) Linecount is
>> an unsigned parameter in these functions. The warning is that the test of linecount >=
>> 0 is always true. The code is trying to ensure that linecount is always within the
>> range of the array. However, if the array is empty (GetCount == 0) then it could
>> subtract 1 from linecount, which would underflow and produce a large positive number,
>> not a negative one, since linecount is unsigned.
>> 
>> 
>> 
>> 
>> 
>> The third patch corrects a warning in pcbnew/modview_frame.cpp that the add operator
>> doesn’t work as intended with a string and an integer. The patch converts the integer
>> to a string prior to the add operator.
>> 
>> 
>> 
>> 
>> 
>> The fourth patch, which I am the least confident of, adds UNSELECTED and UNDEFINED
>> enums to the layer list instead of the macro that coerces -1 and -2 to a LAYER_ID. The
>> warning I am seeing is that equality comparisons of the layer_id to UNDEFINED_LAYER and
>> UNSELECTED_LAYER will never be true. What I am seeing is that the -1 is treated as
>> unsigned, which means it has a value of a large positive number (4294967295). Because
>> the enum has a type of unsigned char, its max value is 255 so the 4294967295 is out of
>> range, hence the warning that they will never compare. To fix this, I created enum
>> values UNSELECTED and UNDEFINED and gave them values of -2 and -1.  The change fixes
>> the warning and doesn’t seem to impact the functioning of the programs, but I don’t
>> know for sure what impact the change has across the entire code base.
>> 
>> 
> 
> 
> 
> On 07/30/2014 02:51 PM, Michael Narigon wrote:>
>> datafile_read_write.patch
>> 
>> 
>> === modified file 'pcb_calculator/datafile_read_write.h'
>> --- pcb_calculator/datafile_read_write.h	2012-04-02 18:11:00 +0000
>> +++ pcb_calculator/datafile_read_write.h	2014-07-30 01:44:30 +0000
>> @@ -1,5 +1,5 @@
>> #ifndef DATAFILE_READ_WRITE_H_
>> -#define PDATAFILE_READ_WRITE_H_
>> +#define DATAFILE_READ_WRITE_H_
>> /*
>>  * This program source code file is part of KiCad, a free EDA CAD application.
>>  *
>> 
>> 
>> 
>> 
>> The second patch fixes some unsigned comparisons in two files with similar logic
> (cvpcb/class_footprints_listbox.cpp and cvpcb/class_library_listbox.cpp.) Linecount is an
> unsigned parameter in these functions. The warning is that the test of linecount >= 0 is
> always true. The code is trying to ensure that linecount is always within the range of the
> array. However, if the array is empty (GetCount == 0) then it could subtract 1 from
> linecount, which would underflow and produce a large positive number, not a negative one,
> since linecount is unsigned.
>> 
>> 
>> class_listbox.patch
>> 
>> 
>> === modified file 'cvpcb/class_footprints_listbox.cpp'
>> --- cvpcb/class_footprints_listbox.cpp	2014-06-04 17:34:23 +0000
>> +++ cvpcb/class_footprints_listbox.cpp	2014-07-29 17:46:20 +0000
>> @@ -60,11 +60,13 @@
>> 
>> void FOOTPRINTS_LISTBOX::SetString( unsigned linecount, const wxString& text )
>> {
>> -    if( linecount >= m_footprintList.Count() )
>> -        linecount = m_footprintList.Count() - 1;
>> -
>> -    if( linecount >= 0 )
>> +    unsigned count = m_footprintList.Count();
>> +    if( count > 0 )
>> +    {
>> +        if( linecount >= count )
>> +            linecount = count - 1;
>>         m_footprintList[linecount] = text;
>> +    }
>> }
> 
> 
> Why, what is the warning?  What is the data type of Count()?
> 
> 
Here is the warning:
[ 97%] Building CXX object cvpcb/CMakeFiles/cvpcb_kiface.dir/class_footprints_listbox.cpp.o
/Users/mnarigon/Projects/kicad_source/kicad.bzr/cvpcb/class_footprints_listbox.cpp:66:19: warning: 
      comparison of unsigned expression >= 0 is always true
      [-Wtautological-compare]
    if( linecount >= 0 )
        ~~~~~~~~~ ^  ~
1 warning generated.

It looks like m_footprintList.Count() returns a size_t.

> 
> 
>> === modified file 'cvpcb/class_library_listbox.cpp'
>> --- cvpcb/class_library_listbox.cpp	2014-06-04 17:34:23 +0000
>> +++ cvpcb/class_library_listbox.cpp	2014-07-29 17:47:47 +0000
>> @@ -60,11 +60,13 @@
>> 
>> void LIBRARY_LISTBOX::SetString( unsigned linecount, const wxString& text )
>> {
>> -    if( linecount >= m_libraryList.Count() )
>> -        linecount = m_libraryList.Count() - 1;
>> -
>> -    if( linecount >= 0 )
>> +    unsigned count = m_libraryList.Count();
>> +    if( count > 0 )
>> +    {
>> +        if( linecount >= count )
>> +            linecount = count - 1;
>>         m_libraryList[linecount] = text;
>> +    }
>> }
> 
> 
> 
> Again, same questions...
> 
> 
>> The third patch corrects a warning in pcbnew/modview_frame.cpp that the add operator
> doesn’t work as intended with a string and an integer. The patch converts the integer to a
> string prior to the add operator.
>> 
>> 
>> modview_frame.patch
>> 
>> 
>> === modified file 'pcbnew/modview_frame.cpp'
>> --- pcbnew/modview_frame.cpp	2014-07-23 10:06:24 +0000
>> +++ pcbnew/modview_frame.cpp	2014-07-29 02:24:54 +0000
>> @@ -713,8 +713,8 @@
>>         break;
>> 
>>     default:
>> -        wxFAIL_MSG( wxT( "FOOTPRINT_VIEWER_FRAME::OnIterateFootprintList error: id = " ) +
>> -                    event.GetId() );
>> +        wxString id = wxString::Format(wxT("%i"),event.GetId());
>> +        wxFAIL_MSG( wxT( "FOOTPRINT_VIEWER_FRAME::OnIterateFootprintList error: id = "
> ) + id );
>>     }
>> }
> 
> 
> OK.
> 
> 
> 
> 
> 
> 
>> The fourth patch, which I am the least confident of, adds UNSELECTED and UNDEFINED enums
> to the layer list instead of the macro that coerces -1 and -2 to a LAYER_ID. The warning I
> am seeing is that equality comparisons of the layer_id to UNDEFINED_LAYER and
> UNSELECTED_LAYER will never be true. What I am seeing is that the -1 is treated as
> unsigned, which means it has a value of a large positive number (4294967295). Because the
> enum has a type of unsigned char, its max value is 255 so the 4294967295 is out of range,
> hence the warning that they will never compare. To fix this, I created enum values
> UNSELECTED and UNDEFINED and gave them values of -2 and -1.  The change fixes the warning
> and doesn’t seem to impact the functioning of the programs, but I don’t know for sure what
> impact the change has across the entire code base.
> 
> 
> 
> No, I passed through that path and rejected it.  Tell the compiler to shut up.
> 
> 

Did you happen to check the logic on a C++11 compiler? There is an ifdef on lines 58-60 of the file include/layers_id_colors_and_visibility.h that sets the type of the enum to unsigned char on a C++11 compiler. I suspect that is the cause of the warning, because unsigned isn’t sign extended when it is promoted. The warning saying that the code may not be doing what you intended to do, which is why I brought it up to the list.

Michael

> 
> 
>> layers_id_colors_and_visibility.patch
>> 
>> 
>> === modified file 'include/layers_id_colors_and_visibility.h'
>> --- include/layers_id_colors_and_visibility.h	2014-07-07 08:48:47 +0000
>> +++ include/layers_id_colors_and_visibility.h	2014-07-29 02:26:37 +0000
>> @@ -59,7 +59,9 @@
>>     : unsigned char
>> #endif
>> {
>> -    F_Cu,           // 0
>> +    UNSELECTED = -2,
>> +    UNDEFINED = -1,
>> +    F_Cu = 0,
>>     In1_Cu,
>>     In2_Cu,
>>     In3_Cu,
>> @@ -121,8 +123,8 @@
>> };
>> 
>> 
>> -#define UNDEFINED_LAYER     LAYER_ID(-1)
>> -#define UNSELECTED_LAYER    LAYER_ID(-2)
>> +#define UNDEFINED_LAYER     LAYER_ID(UNDEFINED)
>> +#define UNSELECTED_LAYER    LAYER_ID(UNSELECTED)
>> 
>> #define MAX_CU_LAYERS       (B_Cu - F_Cu + 1)
> 
> 
> 
> 
> 
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp