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)
_______________________________________________
Mailing list: https://launchpad.net/~kicad-developers
Post to : kicad-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help : https://help.launchpad.net/ListHelp
