arobenko / embxx

embxx - Embedded C++ Library
GNU General Public License v3.0
262 stars 35 forks source link

Fix sign-compare warnings and CMake C++11 #6

Closed ajneu closed 8 years ago

ajneu commented 8 years ago

Hi,

fixed some warnings regarding sign-compare. Also, CMake now has a nice method of settings C++11 (for many different compilers):

https://cmake.org/cmake/help/latest/variable/CMAKE_CXX_STANDARD.html https://cmake.org/cmake/help/latest/variable/CMAKE_CXX_EXTENSIONS.html

So to set C++11, we should use:

set(CMAKE_CXX_STANDARD   11)
set(CMAKE_CXX_EXTENSIONS OFF)  ## no non-standard extensions

Regards, ajneu

arobenko commented 8 years ago

If I understand you correctly CMAKE_CXX_STANDARD and CMAKE_CXX_EXTENSIONS are just variables you have to set, they are not compiler flags. So I think your pull request has an error of passing them as compiler options.

ajneu commented 8 years ago

OK, added a commit for that now.

. . .

By the way: I urge you to run your projects via clang compiler also. I've just done it:

export CC=/usr/bin/clang
export CXX=/usr/bin/clang++

There are numerous errors reported, when using clang: Example:

/home/mememe/projects/embxx/embxx/container/StaticQueue.h:535:29: error: variable has incomplete type 'embxx::container::details::StaticQueueBase::Iterator'
    Iterator erase(Iterator pos)
                            ^
/home/mememe/projects/embxx/embxx/container/StaticQueue.h:54:11: note: forward declaration of 'embxx::container::details::StaticQueueBase::Iterator'
    class Iterator;
ajneu commented 8 years ago

Low prio:

So I think your pull request has an error of passing them as compiler options.

It didn't have an error, by the way. The only difference is that CMAKE_CXX_FLAGS (ref) passes it's arguments directly to the command, while CMAKE_CXX_STANDARD (ref), transforms the argument and then passes it to the command.

(Passing to the command, means that it set's CXXFLAGS accordingly, in the generated Makefile.)

But neither CMAKE_CXX_FLAGS nor CMAKE_CXX_STANDARD are make's CXXFLAGS by name: In both cases their content just get passed to CXXFLAGS (either directly or through a prior transformation).

Edit: but I do find the variant of the 2nd commit (on this pull-request) clearer myself - link.

arobenko commented 8 years ago

Hi Albert, I've merged your fixes and added a couple of my own, mostly fixes reported when "-Wshadow" is used. As for "clang", it's been on my TODO list for a while, but I doubt I'll have time to look at it in the near future, not until May anyway. I suppose I will attend to it at the same time when supporting gcc5 (issue #5). You may open a separate issue for it if you wish.