kicad-developers team mailing list archive

[Dick Hollenbeck <dick@xxxxxxxxxxx>]

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()?




> === 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.




> 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)