Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.9.2
    • Labels:
      None
    • Patch Info:
      Patch Available

      Description

      I would like to introduce additional compiler flags to improve the C++ code quality:

      -CXXFLAGS = -Wall
      +CXXFLAGS = -Wall -Wextra -std=c++0x -pedantic
      

        Issue Links

          Activity

          Hide
          Roger Meier added a comment -

          any concerns?

          Show
          Roger Meier added a comment - any concerns?
          Hide
          Jake Farrell added a comment -

          only concern would be breaking older compilers, if we can document a starting version and above for a supported compiler i'm good with this

          Show
          Jake Farrell added a comment - only concern would be breaking older compilers, if we can document a starting version and above for a supported compiler i'm good with this
          Hide
          Roger Meier added a comment -

          I would say gcc >= 4.4.5 is the starting point, older ones should work as well. I can roll back if users have issues with other platforms or compilers.
          From my perspective, this setting should be suitable for most modern compilers and brings use to a C++ standard conform implementation across all platforms.

          Show
          Roger Meier added a comment - I would say gcc >= 4.4.5 is the starting point, older ones should work as well. I can roll back if users have issues with other platforms or compilers. From my perspective, this setting should be suitable for most modern compilers and brings use to a C++ standard conform implementation across all platforms.
          Hide
          Roger Meier added a comment -

          committed!

          Show
          Roger Meier added a comment - committed!
          Hide
          Jake Farrell added a comment -

          This break centos 5.x since gcc version is 4.1.2 by default. Centos 6.x is fine since the default version is 4.4.4.

          This also breaks OS X snow leopard which is gcc 4.2.1.

          And the CI also failed for Job 233 for this under Thrift-Compiler-Windows.

          Show
          Jake Farrell added a comment - This break centos 5.x since gcc version is 4.1.2 by default. Centos 6.x is fine since the default version is 4.4.4. This also breaks OS X snow leopard which is gcc 4.2.1. And the CI also failed for Job 233 for this under Thrift-Compiler-Windows.
          Hide
          Hudson added a comment -

          Integrated in Thrift #363 (See https://builds.apache.org/job/Thrift/363/)
          THRIFT-1462 add more strict compiler flags(-Wall -Wextra -std=c++0x -pedantic)

          roger : http://svn.apache.org/viewvc/?view=rev&rev=1213459
          Files :

          • /thrift/trunk/compiler/cpp/Makefile.am
          • /thrift/trunk/lib/cpp/Makefile.am
          • /thrift/trunk/lib/cpp/test/Makefile.am
          • /thrift/trunk/test/cpp/Makefile.am
          Show
          Hudson added a comment - Integrated in Thrift #363 (See https://builds.apache.org/job/Thrift/363/ ) THRIFT-1462 add more strict compiler flags(-Wall -Wextra -std=c++0x -pedantic) roger : http://svn.apache.org/viewvc/?view=rev&rev=1213459 Files : /thrift/trunk/compiler/cpp/Makefile.am /thrift/trunk/lib/cpp/Makefile.am /thrift/trunk/lib/cpp/test/Makefile.am /thrift/trunk/test/cpp/Makefile.am
          Hide
          Roger Meier added a comment -

          just committed a fix for MINGW

          if MINGW
          thrift_CXXFLAGS = -Wall
          else
          thrift_CXXFLAGS = -Wall -Wextra -std=c++0x -pedantic
          endif
          

          we probably need more of these checks

          Show
          Roger Meier added a comment - just committed a fix for MINGW if MINGW thrift_CXXFLAGS = -Wall else thrift_CXXFLAGS = -Wall -Wextra -std=c++0x -pedantic endif we probably need more of these checks
          Hide
          Jake Farrell added a comment -

          Instead of adding these checks all over it would probably be better to add one configure check against the compiler version and then add them if its greater than 4.4

          Show
          Jake Farrell added a comment - Instead of adding these checks all over it would probably be better to add one configure check against the compiler version and then add them if its greater than 4.4
          Hide
          Hudson added a comment -

          Integrated in Thrift #364 (See https://builds.apache.org/job/Thrift/364/)
          THRIFT-1462 add more strict compiler flags
          FIX: check for MINGW and use reduced compiler flag set for mingw

          roger : http://svn.apache.org/viewvc/?view=rev&rev=1213523
          Files :

          • /thrift/trunk/compiler/cpp/Makefile.am
          • /thrift/trunk/configure.ac
          Show
          Hudson added a comment - Integrated in Thrift #364 (See https://builds.apache.org/job/Thrift/364/ ) THRIFT-1462 add more strict compiler flags FIX: check for MINGW and use reduced compiler flag set for mingw roger : http://svn.apache.org/viewvc/?view=rev&rev=1213523 Files : /thrift/trunk/compiler/cpp/Makefile.am /thrift/trunk/configure.ac
          Hide
          Jake Farrell added a comment -

          Reverted patch as it is breaking build for current systems with default compiler less than 4.4

          Show
          Jake Farrell added a comment - Reverted patch as it is breaking build for current systems with default compiler less than 4.4
          Hide
          Hudson added a comment -

          Integrated in Thrift #366 (See https://builds.apache.org/job/Thrift/366/)
          Thrift-1462: add more strict compiler flags

          Reverting patch due to it breaking compilers older than 4.4 (os x, centos 5.x, etc)

          Show
          Hudson added a comment - Integrated in Thrift #366 (See https://builds.apache.org/job/Thrift/366/ ) Thrift-1462: add more strict compiler flags Reverting patch due to it breaking compilers older than 4.4 (os x, centos 5.x, etc)
          Hide
          Hudson added a comment -

          Integrated in Thrift #367 (See https://builds.apache.org/job/Thrift/367/)
          Thrift-1462: add more strict compiler flags

          Reverting MINGW addition for c+++0x

          Show
          Hudson added a comment - Integrated in Thrift #367 (See https://builds.apache.org/job/Thrift/367/ ) Thrift-1462: add more strict compiler flags Reverting MINGW addition for c+++0x
          Hide
          Roger Meier added a comment -

          Jake, I fully agree with that!

          People should be able to use a regular distro with default packages to run Thrift.
          Just enabling compiler switch if option is available is the right direction.

          I will have a closer look at compiler(gcc,llvm) and version detection macros as soon as I have some time to do so...
          contributors, patches are also welcome here.

          Show
          Roger Meier added a comment - Jake, I fully agree with that! People should be able to use a regular distro with default packages to run Thrift. Just enabling compiler switch if option is available is the right direction. I will have a closer look at compiler(gcc,llvm) and version detection macros as soon as I have some time to do so... contributors, patches are also welcome here.
          Hide
          Roger Meier added a comment -

          What about adding -Wextra -pedantic ? They are supported by gcc and clang.

          Show
          Roger Meier added a comment - What about adding -Wextra -pedantic ? They are supported by gcc and clang.
          Hide
          Konrad Grochowski added a comment -

          In my branch for new c++ generator I've enabled -Wextra, fixed all warnings and turned on -Werror

          No -Wpedantic yet though

          (see https://github.com/hcorg/thrift/commit/6432a197e4080202b3252ec56f071427d477de74)

          Show
          Konrad Grochowski added a comment - In my branch for new c++ generator I've enabled -Wextra, fixed all warnings and turned on -Werror No -Wpedantic yet though (see https://github.com/hcorg/thrift/commit/6432a197e4080202b3252ec56f071427d477de74 )
          Hide
          Roger Meier added a comment -

          committed -Wextra -pedantic plus some minor fixes.

          only exception is lib/cpp/test => -pedantic breaks the build

          Show
          Roger Meier added a comment - committed -Wextra -pedantic plus some minor fixes. only exception is lib/cpp/test => -pedantic breaks the build
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in Thrift #1312 (See https://builds.apache.org/job/Thrift/1312/)
          THRIFT-1462 add more strict compiler flags (roger: rev ec300e33d49bc57d8cc3b744ea16e5af615c8f94)

          • compiler/cpp/Makefile.am
          • compiler/cpp/CMakeLists.txt
          • tutorial/cpp/Makefile.am
          • test/cpp/Makefile.am
          • compiler/cpp/src/generate/t_lua_generator.cc
          • lib/cpp/Makefile.am
          • compiler/cpp/src/generate/t_delphi_generator.cc
          • lib/cpp/test/Makefile.am
          Show
          Hudson added a comment - SUCCESS: Integrated in Thrift #1312 (See https://builds.apache.org/job/Thrift/1312/ ) THRIFT-1462 add more strict compiler flags (roger: rev ec300e33d49bc57d8cc3b744ea16e5af615c8f94) compiler/cpp/Makefile.am compiler/cpp/CMakeLists.txt tutorial/cpp/Makefile.am test/cpp/Makefile.am compiler/cpp/src/generate/t_lua_generator.cc lib/cpp/Makefile.am compiler/cpp/src/generate/t_delphi_generator.cc lib/cpp/test/Makefile.am
          Hide
          Hudson added a comment -

          SUCCESS: Integrated in Thrift #1331 (See https://builds.apache.org/job/Thrift/1331/)
          THRIFT-1462 add more strict compiler flags (roger: rev ec300e33d49bc57d8cc3b744ea16e5af615c8f94)

          • tutorial/cpp/Makefile.am
          • lib/cpp/test/Makefile.am
          • compiler/cpp/Makefile.am
          • test/cpp/Makefile.am
          • compiler/cpp/src/generate/t_lua_generator.cc
          • lib/cpp/Makefile.am
          • compiler/cpp/src/generate/t_delphi_generator.cc
          • compiler/cpp/CMakeLists.txt
          Show
          Hudson added a comment - SUCCESS: Integrated in Thrift #1331 (See https://builds.apache.org/job/Thrift/1331/ ) THRIFT-1462 add more strict compiler flags (roger: rev ec300e33d49bc57d8cc3b744ea16e5af615c8f94) tutorial/cpp/Makefile.am lib/cpp/test/Makefile.am compiler/cpp/Makefile.am test/cpp/Makefile.am compiler/cpp/src/generate/t_lua_generator.cc lib/cpp/Makefile.am compiler/cpp/src/generate/t_delphi_generator.cc compiler/cpp/CMakeLists.txt

            People

            • Assignee:
              Roger Meier
              Reporter:
              Roger Meier
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development