Added build flag to disable pedantic builds #13256
Conversation
CMakeLists.txt
Outdated
| @@ -35,6 +35,7 @@ endif() | |||
| option(SOLC_LINK_STATIC "Link solc executable statically on supported platforms" OFF) | |||
| option(SOLC_STATIC_STDLIBS "Link solc against static versions of libgcc and libstdc++ on supported platforms" OFF) | |||
| option(STRICT_Z3_VERSION "Use the latest version of Z3" ON) | |||
| option(DISABLE_WARNINGS "Disable all warning flags, treating warnings as errors and all other pedantic build flags" OFF) | |||
There was a problem hiding this comment.
I think we should add a note about this flag to the docs page about building from source.
|
Thank you for the detailed review! |
CMakeLists.txt
Outdated
| @@ -48,6 +49,9 @@ include_directories(SYSTEM ${JSONCPP_INCLUDE_DIR}) | |||
|
|
|||
| find_package(Threads) | |||
|
|
|||
| if(NOT PEDANTIC) | |||
| message("-- Pedantic build flags turned off. Warnings will not make compilation fail. This is NOT recommended in development builds.") | |||
There was a problem hiding this comment.
"This is NOT recommended in development builds."
Shouldn't that NOT be recommended at all? It should probably say instead that this option should be only enabled when you know what you are doing - as in - "use with care!". What do you think?
There was a problem hiding this comment.
IMO it really only matters for development. These are warnings, not errors and on top of that only those that don't happen in most configurations (or we'd catch them in CI) so it's unlikely that they're harmful. The user building a release cannot do anything about them anyway.
Maybe we could just say that we recommend reporting warnings as issues in the repo?
There was a problem hiding this comment.
Recommending to report an issue to us sounds good. Also unconditionally stating "This is NOT recommended." is ok with me, but also fine as is.
But we should at least make it a cmake warning, i.e. message(WARNING "....
|
The PR looks fine to me in the current form but the last thing that is missing is the extra bit of docs (#13256 (comment)). Other than that, we could merge it right away. |
cameel
dismissed
their stale review
Some stuff is missing but there are no issues to be fixed
|
I added a bit of docs about the new flag. The PR is complete now. |
Added build flag
DISABLE_WARNINGSwhich is OFF by default and disables warnings.Potentially closes #13146