Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CMake Cleanup Part 2 #472

Closed
7 tasks done
stonier opened this issue Jun 17, 2018 · 4 comments
Closed
7 tasks done

CMake Cleanup Part 2 #472

stonier opened this issue Jun 17, 2018 · 4 comments
Assignees
Milestone

Comments

@stonier
Copy link
Collaborator

stonier commented Jun 17, 2018

@stonier stonier added this to the Milestone #3 milestone Jun 17, 2018
@stonier stonier self-assigned this Jun 17, 2018
This was referenced Jun 17, 2018
@apojomovsky
Copy link

Hi @stonier, I'm currently looking to finish this issue so that we can finally close it.
May I ask you a brief explanation on what's expected for the remaining item "Review the CFlags maze"?
Thanks in advance!

@stonier
Copy link
Collaborator Author

stonier commented Sep 18, 2018

I simply hadn't parsed it yet. As with other parts of the CMake it is likely way overkill for our needs. Keeping it simple will foster easier maintenance going forward.

@clalancette
Copy link
Contributor

Going through the cmake stuff in Delphyne, I see the following that tries to add compiler flags:

  • CMakeLists.txt at the top
    • Includes cmake/Utils.cmake
      • Utils.cmake actually defines the filter_valid_compiler_warnings macro that the top-level CMakeLists.txt uses to see if the compiler supports certain warning flags
    • Includes cmake/HostCFlags.cmake
      • HostCFlags includes FindSSE.cmake
      • If FindSSE.cmake find SSE, then HostCFlags adds the SSE flags to CMAKE_C_FLAGS_ALL.
    • Includes cmake/DefaultCFlags.cmake
      • This adds in a number of flags, depending on the CMAKE_BUILD_TYPE (things like debugging, profiling, etc).
      • This also currently adds in -rdynamic to the link flags, though that is no longer needed.
    • Attempts to add flags on its own using the filter_valid_compiler_warnings macro from Utils.
  • Looking through the rest of the CMakeLists.txt in the delphyne repository, nothing else actually adds flags, so the above is the full list of what touches flags.

With that in mind, I'm not sure exactly what we could simplify. We could just assume that we are always on g++ on an amd64 machine, which would simplify some of this machinery, but it has been useful at times in the past to compile with clang. As far as -rdynamic goes, we no longer need that, so I'll open a PR removing those flags. Otherwise, I'm not sure that we can change too much here, other than moving flags from the top-level CMakeLists.txt so that they are hidden in one of the cmake/* files. @stonier , thoughts?

@clalancette
Copy link
Contributor

Since all of the items here are now checked off, I'm going to close this out. Feel free to reopen (or open a new issue) if there is anything missing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants