kicad-developers team mailing list archive

[Chris Pavlina <pavlina.chris@xxxxxxxxx>]

It is indeed difficult to test an issue that doesn't appear on your computer, but from a quick look around, it looks like the correct way is to leave std::abs, but add #include <cstdlib>. Unlike stdlib.h, cstdlib defines overloads for all the integer widths, reducing the possibility of a bad mistake when using it with something longer than an int:

http://www.cplusplus.com/reference/cstdlib/abs/

Perhaps an oversight on my part not to have included that in the first place. Maybe you could give that a try? The usage would be much more obvious then.


On Mon, Dec 07, 2015 at 02:17:19PM +0100, jp charras wrote:
> Le 07/12/2015 13:51, Chris Pavlina a écrit :
> > Mostly directed at JP:
> > 
> > Back in June, I went through and changed a few 'weird' things we were doing, including using C abs instead of C++ std::abs:
> > 
> > http://bazaar.launchpad.net/~kicad-product-committers/kicad/product/revision/5831
> > 
> > You just changed it back:
> > 
> > http://bazaar.launchpad.net/~kicad-product-committers/kicad/product/revision/6350
> > 
> > What compiler could possibly have an issue with that? std::abs is, for very good reason, the preferred way to find absolute value in C++. As such, if there is some obscure reason we need to use abs(), would it kill you to leave some explanation behind so nobody else also gets confused by that? A real explanation in the commit message would be extremely helpful, as possibly would a comment in the vicinity of the intentionally nonstandard code.
> > 
> > --
> > Chris
> 
> 
> std::abs is preferable to std, and I had no issue on my computer.
> It is not so easy to fix an issue (using std::abs with integers,
> std::abs used with floats did not created issues) which does not appears
> on your computer.
> 
> But here is my reason:
> 
> > See <http://ci.kicad-pcb.org/job/kicad-full/947/changes>
> > 
> > Changes:
> > 
> > [jean-pierre charras] Kicad manager: fix a potential bug which could crash Kicad manager. math_for_graphics.cpp: remove useless includes.
> > 
> > ------------------------------------------
> > Started by an SCM change
> > Building on master in workspace <http://ci.kicad-pcb.org/job/kicad-full/ws/>
> > $ bzr revision-info -d <http://ci.kicad-pcb.org/job/kicad-full/ws/>
> > info result: bzr revision-info -d <http://ci.kicad-pcb.org/job/kicad-full/ws/> returned 0. Command output: "6348 jp.charras@xxxxxxxxxx-20151207092209-7rby34qymt287ho6
> > " stderr: ""
> > [kicad-full] $ bzr pull --overwrite lp:kicad
> > You have not informed bzr of your Launchpad ID, and you must do this to
> > write to Launchpad or access private data.  See "bzr help launchpad-login".
> > http://bazaar.launchpad.net/~kicad-product-committers/kicad/product is permanently redirected to http://bazaar.launchpad.net/~kicad-product-committers/kicad/product/changes
> > You have not informed bzr of your Launchpad ID, and you must do this to
> > write to Launchpad or access private data.  See "bzr help launchpad-login".
> >  M  kicad/class_treeproject_item.cpp
> >  M  polygon/math_for_graphics.cpp
> > All changes applied successfully.
> > Now on revision 6349.
> > [kicad-full] $ bzr revert
> > $ bzr revision-info -d <http://ci.kicad-pcb.org/job/kicad-full/ws/>
> > info result: bzr revision-info -d <http://ci.kicad-pcb.org/job/kicad-full/ws/> returned 0. Command output: "6349 jp.charras@xxxxxxxxxx-20151207092431-90sr06sy0jhwwg2f
> > " stderr: ""
> > [kicad-full] $ bzr log -v -r revid:jp.charras@xxxxxxxxxx-20151207092209-7rby34qymt287ho6..revid:jp.charras@xxxxxxxxxx-20151207092431-90sr06sy0jhwwg2f --long --show-ids
> > Getting local revision...
> > $ bzr revision-info -d <http://ci.kicad-pcb.org/job/kicad-full/ws/>
> > info result: bzr revision-info -d <http://ci.kicad-pcb.org/job/kicad-full/ws/> returned 0. Command output: "6349 jp.charras@xxxxxxxxxx-20151207092431-90sr06sy0jhwwg2f
> > " stderr: ""
> > RevisionState revno:6349 revid:jp.charras@xxxxxxxxxx-20151207092431-90sr06sy0jhwwg2f
> > [kicad-full] $ /bin/sh -xe /tmp/hudson8903420887098700890.sh
> > + OPTS=' -DCMAKE_BUILD_TYPE=Debug -DBUILD_GITHUB_PLUGIN=ON -DKICAD_SCRIPTING=ON -DKICAD_SCRIPTING_MODULES=ON -DKICAD_SCRIPTING_WXPYTHON=ON'
> > + '[' -d build ']'
> > + cd build
> > + /usr/bin/cmake .. -DCMAKE_BUILD_TYPE=Debug -DBUILD_GITHUB_PLUGIN=ON -DKICAD_SCRIPTING=ON -DKICAD_SCRIPTING_MODULES=ON -DKICAD_SCRIPTING_WXPYTHON=ON
> > -- Kicad install dir: </usr/local>
> > -- Check for installed OpenGL -- found
> > -- Found Glew: /usr/lib64/libGLEW.so
> > -- Check for installed GLEW -- found
> > -- Check for installed Cairo -- found
> > -- Check for installed Python Interpreter -- found
> > -- Python module install path: lib/python2.6/site-packages
> > -- wxPython version 3.0 found.
> > -- Configuring done
> > -- Generating done
> > -- Build files have been written to: <http://ci.kicad-pcb.org/job/kicad-full/ws/build>
> > + rm -f pcbnew/scripting/pcbnewPYTHON_wrap.cxx.o
> > + rm -f pcbnew/scripting/pcbnewPYTHON_wrap.cxx
> > + make -j4
> > [  0%] Built target page_layout_lexer_source_files
> > [  1%] Built target boost
> > [  1%] Generating version string header
> > [  1%] Built target netlist_lexer_source_files
> > -- Using Bazaar to determine build version string.
> > [  1%] Built target fp_lib_table_lexer_source_files
> > [  1%] Generating headers containing GLSL source code
> > [  1%] Built target pcb_lexer_source_files
> > Headers are up-to-date
> > [  1%] Built target shader_headers
> > [  1%] [  1%] Built target specctra_lexer_source_files
> > Built target pcb_plot_lexer_source_files
> > [  1%] Built target cmp_library_lexer_source_files
> > [  1%] Built target avhttp
> > [  1%] Built target dialog_bom_cfg_lexer_source_files
> > [  1%] Built target field_template_lexer_source_files
> > -- Bazaar version control system version 2.1.1 found.
> > [  2%] Built target lib_dxf
> > [  2%] Built target idf3
> > [  3%] Built target potrace
> > [  3%] Built target pcb_calculator_lexer_source_files
> > [  3%] Built target dxf2idf
> > [ 32%] [ 32%] Built target bitmaps
> > Built target idfcyl
> > [ 32%] Built target lib-dependencies
> > [ 32%] Built target idfrect
> > [ 32%] Built target idf2vrml
> > -- Writing <http://ci.kicad-pcb.org/job/kicad-full/ws/build/kicad_build_version.h> file with version: (2015-12-07 BZR 6349)
> > [ 32%] Built target version_header
> > Scanning dependencies of target polygon
> > [ 32%] Building CXX object polygon/CMakeFiles/polygon.dir/math_for_graphics.cpp.o
> > [ 34%] Built target gal
> > [ 34%] Built target github_plugin
> > <http://ci.kicad-pcb.org/job/kicad-full/ws/polygon/math_for_graphics.cpp>: In function ‘double GetPointToLineSegmentDistance(int, int, int, int, int, int)’:
> > <http://ci.kicad-pcb.org/job/kicad-full/ws/polygon/math_for_graphics.cpp>:467: error: call of overloaded ‘abs(int)’ is ambiguous
> > /usr/lib/gcc/x86_64-redhat-linux/4.4.7/../../../../include/c++/4.4.7/cmath:94: note: candidates are: double std::abs(double)
> > /usr/lib/gcc/x86_64-redhat-linux/4.4.7/../../../../include/c++/4.4.7/cmath:98: note:                 float std::abs(float)
> > /usr/lib/gcc/x86_64-redhat-linux/4.4.7/../../../../include/c++/4.4.7/cmath:102: note:                 long double std::abs(long double)
> > <http://ci.kicad-pcb.org/job/kicad-full/ws/polygon/math_for_graphics.cpp>:475: error: call of overloaded ‘abs(int)’ is ambiguous
> > /usr/lib/gcc/x86_64-redhat-linux/4.4.7/../../../../include/c++/4.4.7/cmath:94: note: candidates are: double std::abs(double)
> > /usr/lib/gcc/x86_64-redhat-linux/4.4.7/../../../../include/c++/4.4.7/cmath:98: note:                 float std::abs(float)
> > /usr/lib/gcc/x86_64-redhat-linux/4.4.7/../../../../include/c++/4.4.7/cmath:102: note:                 long double std::abs(long double)
> > make[2]: *** [polygon/CMakeFiles/polygon.dir/math_for_graphics.cpp.o] Error 1
> > make[1]: *** [polygon/CMakeFiles/polygon.dir/all] Error 2
> > make[1]: *** Waiting for unfinished jobs....
> > [ 37%] Built target pcbcommon
> > Scanning dependencies of target common
> > [ 37%] Building CXX object common/CMakeFiles/common.dir/build_version.cpp.o
> > Linking CXX static library libcommon.a
> > [ 44%] Built target common
> > make: *** [all] Error 2
> > Build step 'Execute shell' marked build as failure
> > [WARNINGS] Skipping publisher since build result is FAILURE
> 
> 
> 
> -- 
> Jean-Pierre CHARRAS
> 
> _______________________________________________
> 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