Details

    • Type: Sub-task Sub-task
    • Status: Reopened
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: 1.0
    • 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 -

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

          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
          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
          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
          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
          Roger Meier added a comment -

          committed!

          Show
          Roger Meier added a comment - committed!
          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
          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 -

          any concerns?

          Show
          Roger Meier added a comment - any concerns?

            People

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

              Dates

              • Created:
                Updated:

                Development