cmake: Check -Wno-* compiler options for leveldb target #31366

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:241125-nowarn changing 1 files +5 −3
  1. hebasto commented at 3:12 pm on November 25, 2024: member

    Otherwise, https://cirrus-ci.com/task/4830737755537408:

    0At global scope:
    1cc1plus: note: unrecognized command-line option ‘-Wno-conditional-uninitialized’ may have been intended to silence earlier diagnostics
    
  2. hebasto added the label Build system on Nov 25, 2024
  3. DrahtBot commented at 3:12 pm on November 25, 2024: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31366.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK TheCharlatan
    Stale ACK theuni

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  4. theuni approved
  5. theuni commented at 6:08 pm on November 25, 2024: member

    utACK 0f73d9dd49b371c28ba9e74ef3519ef8f6e80ebc

    A more helpful commit message would’ve been useful though: “Check for -Wfoo rather than -Wno-foo because the latter may not cause the test to fail”.

  6. cmake: Check `-Wno-*` compiler options for `leveldb` target
    Check for -Wfoo rather than -Wno-foo because the latter may not cause
    the test to fail.
    9e4a4b4832
  7. hebasto force-pushed on Nov 25, 2024
  8. hebasto commented at 8:25 pm on November 25, 2024: member

    A more helpful commit message would’ve been useful though: “Check for -Wfoo rather than -Wno-foo because the latter may not cause the test to fail”.

    Thanks! The commit message has been amended per your feedback.

  9. DrahtBot added the label CI failed on Nov 25, 2024
  10. DrahtBot removed the label CI failed on Nov 26, 2024
  11. TheCharlatan approved
  12. TheCharlatan commented at 9:51 am on November 26, 2024: contributor
    ACK 9e4a4b4832219d9d11da441779ab8a3b1304bd8b
  13. DrahtBot requested review from theuni on Nov 26, 2024
  14. fanquake commented at 12:20 pm on November 27, 2024: member

    In Autotools we filtered out the unwanted flags, ifor CMake, the -Wno-* variants were added (but in a way that didn’t quite work), and this PR fixes their usage by adding checks for the flags. However that means we duplicate checks that we do for the same flags earlier in the buildsystem, and we end up with something like:

    clang++ ... -Wconditional-uninitialized -Wsuggest-override ... -Wno-conditional-uninitialized -Wno-suggest-override

    when building leveldb. Wondering if we could have less duplication, and a more sensical compile command, by reverting to what we did in Autotools, and just filter out the unwanted flags?

  15. hebasto commented at 1:08 pm on November 27, 2024: member

    However that means we duplicate checks that we do for the same flags earlier in the buildsystem…

    Checks are cached. You can observe only a single instance of:

    0-- Performing Test CXX_SUPPORTS__WSUGGEST_OVERRIDE
    1-- Performing Test CXX_SUPPORTS__WSUGGEST_OVERRIDE - Success
    

    in the configuration output.

  16. fanquake commented at 1:10 pm on November 27, 2024: member
    The code is duplicated.
  17. maflcko commented at 1:20 pm on November 27, 2024: member
    Personally I think it is fine append -Wno... in 5 lines of code. I’d presume filtering would require even more code?
  18. fanquake commented at 1:22 pm on November 27, 2024: member

    I’d presume filtering would require even more code?

    I don’t see why it’d be more than 1 line.


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-03 18:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me