build: avoid repetitions when enabling warnings in configure.ac #18857

pull vasild wants to merge 1 commits into bitcoin:master from vasild:avoid_repetitions_in_configure.ac changing 1 files +35 −30
  1. vasild commented at 2:19 PM on May 3, 2020: contributor

    Introduce a macro APPEND_FLAG_IF_AVAILABLE() and use that.

  2. MarcoFalke added the label Refactoring on May 3, 2020
  3. MarcoFalke commented at 2:26 PM on May 3, 2020: member

    Concept ACK b26f8a0bb9bc6bb1066291e6752b7e33bea89cf9

  4. vasild cross-referenced this on May 3, 2020 from issue build: warn on potentially uninitialized reads by vasild
  5. brakmic commented at 2:57 PM on May 3, 2020: contributor

    Concept ACK b26f8a0bb9bc6bb1066291e6752b7e33bea89cf9

  6. practicalswift commented at 7:29 PM on May 3, 2020: contributor

    Concept ACK

    Very nice initiative: this repetition has annoyed me :)

  7. DrahtBot commented at 11:06 PM on May 3, 2020: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #19015 (build: Enable some commonly enabled compiler diagnostics by practicalswift)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  8. in configure.ac:370 in b26f8a0bb9 outdated 
     332 | @@ -333,31 +333,36 @@ if test x$use_sanitizers != x; then
 333 |      ]],[[]])])
 334 |  fi
 335 |  
 336 | +dnl APPEND_FLAG_IF_AVAILABLE([APPEND_TO_THIS_VARIABLE], [-WallExtra])
 337 | +AC_DEFUN([APPEND_FLAG_IF_AVAILABLE], [
    promag commented at 11:23 PM on May 3, 2020:

    Could also define SET_FLAG_IF_AVAILABLE and use throughout.

    vasild commented at 6:10 AM on May 4, 2020:

    That would be a little bit less straight forward. While such code would definitely benefit from it:

    AX_CHECK_COMPILE_FLAG([-msse4.2],[[SSE42_CXXFLAGS="-msse4.2"]],,[[$CXXFLAG_WERROR]])

    and become:

    SET_FLAG_IF_AVAILABLE([SSE42_CXXFLAGS], [-msse4.2])

    there are other usages where the code calls AC_MSG_ERROR() if the flag is not available or does not compile with an additional $CXXFLAG_WERROR.

    I will consider this as a separate PR.

    promag commented at 12:15 AM on May 5, 2020:

    Yeah didn't check if all follow the same pattern. Separate PR is fine.

  9. promag commented at 11:25 PM on May 3, 2020: member

    Concept ACK.

  10. DrahtBot cross-referenced this on May 4, 2020 from issue test, build: Enable -Werror=sign-compare by Empact
  11. DrahtBot cross-referenced this on May 4, 2020 from issue build: ensure we aren't using GNU extensions by fanquake
  12. DrahtBot cross-referenced this on May 4, 2020 from issue build: Enable -Wsuggest-override if available by hebasto
  13. DrahtBot added the label Needs rebase on May 5, 2020
  14. fanquake added the label Build system on May 5, 2020
  15. fanquake commented at 7:35 AM on May 5, 2020: member

    Concept ~0. Saving same verbosity is nice, but the downsides are now having a macro that obscures what's happening, and is applied inconsistently throughout configure; less than half of our AX_CHECK_COMPILE_FLAG calls are replaced with it.

    This approach would also not work with the change currently proposed in #18882 (we'd end up with duplicate -Wformat flags). So let's decide how we want to fix the issue in that PR, before merging something like this.

  16. vasild force-pushed on May 5, 2020
  17. DrahtBot removed the label Needs rebase on May 5, 2020
  18. DrahtBot cross-referenced this on May 5, 2020 from issue build: enable -Werror=gnu by vasild
  19. DrahtBot cross-referenced this on May 5, 2020 from issue build: fix -Wformat-security check when compiling with GCC by fanquake
  20. DrahtBot added the label Needs rebase on May 6, 2020
  21. vasild force-pushed on May 6, 2020
  22. DrahtBot removed the label Needs rebase on May 6, 2020
  23. DrahtBot cross-referenced this on May 9, 2020 from issue Replace -Wthread-safety-analysis with broader -Wthread-safety by hebasto
  24. DrahtBot added the label Needs rebase on May 11, 2020
  25. vasild force-pushed on May 11, 2020
  26. vasild commented at 9:22 AM on May 11, 2020: contributor

    Rebased.

  27. DrahtBot removed the label Needs rebase on May 11, 2020
  28. DrahtBot added the label Needs rebase on May 13, 2020
  29. vasild force-pushed on May 13, 2020
  30. vasild commented at 12:32 PM on May 13, 2020: contributor

    Rebased.

  31. DrahtBot removed the label Needs rebase on May 13, 2020
  32. DrahtBot added the label Needs rebase on May 13, 2020
  33. vasild force-pushed on May 21, 2020
  34. vasild commented at 8:15 AM on May 21, 2020: contributor

    Rebased.

  35. DrahtBot removed the label Needs rebase on May 21, 2020
  36. DrahtBot cross-referenced this on May 22, 2020 from issue build: Enable some commonly enabled compiler diagnostics by practicalswift
  37. DrahtBot added the label Needs rebase on May 28, 2020
  38. vasild force-pushed on May 28, 2020
  39. vasild commented at 8:00 PM on May 28, 2020: contributor

    Rebased.

  40. DrahtBot removed the label Needs rebase on May 28, 2020
  41. DrahtBot cross-referenced this on Jun 2, 2020 from issue refactor: Fix unreachable code in init arg checks by jonathanschoeller
  42. vasild force-pushed on Jun 4, 2020
  43. vasild commented at 10:49 AM on June 4, 2020: contributor

    Rebased.

  44. DrahtBot added the label Needs rebase on Jun 4, 2020
  45. vasild force-pushed on Jun 4, 2020
  46. vasild commented at 7:26 PM on June 4, 2020: contributor

    Rebased.

  47. DrahtBot removed the label Needs rebase on Jun 4, 2020
  48. DrahtBot cross-referenced this on Jun 6, 2020 from issue test: Avoid overwriting the NodeContext member of the testing setup [-Wshadow-field] by MarcoFalke
  49. DrahtBot added the label Needs rebase on Jun 8, 2020
  50. vasild force-pushed on Jun 8, 2020
  51. vasild commented at 7:32 PM on June 8, 2020: contributor

    Rebased.

  52. DrahtBot removed the label Needs rebase on Jun 8, 2020
  53. DrahtBot added the label Needs rebase on Jul 15, 2020
  54. build: avoid repetitions when enabling warnings
    Introduce a macro APPEND_FLAG_IF_AVAILABLE() and use that.
    2a7275827a
  55. vasild force-pushed on Jul 16, 2020
  56. vasild commented at 8:53 AM on July 16, 2020: contributor

    Rebased due to conflicts. @fanquake, what's your take on this, now that #18882 has been merged?

  57. DrahtBot removed the label Needs rebase on Jul 16, 2020
  58. fanquake commented at 10:24 AM on August 10, 2020: member

    @fanquake, what's your take on this, now that #18882 has been merged?

    My take is still the same as my comment above. I'm not convinced introducing an inconsistently used macro, to essentially save some line length, is a huge win. In general I prefer that we are explicit/verbose when it comes to anything build system related.

  59. DrahtBot added the label Needs rebase on Aug 18, 2020
  60. DrahtBot commented at 4:36 AM on August 18, 2020: contributor

    <!--cf906140f33d8803c4a75a2196329ecb-->

    🐙 This pull request conflicts with the target branch and needs rebase.

    <sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>

  61. vasild commented at 7:03 AM on August 18, 2020: contributor

    Closing this because it needs frequent rebasing and it is not moving anywhere.

    It has 4 "Concept ACK" and 1 "Concept ~0".

    The point of this PR is to save repetitions, not line length. The introduced macro is not used everywhere (to replace AX_CHECK_COMPILE_FLAG), but on the places where it is used the usage is consistent.

    If there is interest in this, I would be happy to reopen and rebase it.

  62. vasild closed this on Aug 18, 2020
  63. hebasto commented at 7:11 AM on August 18, 2020: member

    I tend to agree with @fanquake:

    In general I prefer that we are explicit/verbose when it comes to anything build system related.

  64. fanquake cross-referenced this on Mar 18, 2021 from issue build: Add convenient BITCOIN_TRY_ADD_COMPILE_FLAG macro by hebasto
  65. bitcoin locked this on Feb 15, 2022
Contributors
vasildMarcoFalkebrakmicpracticalswiftDrahtBotpromagfanquakehebasto
Labels
RefactoringBuild systemNeeds rebase
Linked (view graph)
#18843 build: warn on potentially uninitialized reads#18882 build: fix -Wformat-security check when compiling with GCC#21459 build: Add convenient BITCOIN_TRY_ADD_COMPILE_FLAG macro
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: 2026-05-19 06:54 UTC