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: member
    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. brakmic commented at 2:57 pm on May 3, 2020: contributor
    Concept ACK b26f8a0bb9bc6bb1066291e6752b7e33bea89cf9
  5. practicalswift commented at 7:29 pm on May 3, 2020: contributor

    Concept ACK

    Very nice initiative: this repetition has annoyed me :)

  6. DrahtBot commented at 11:06 pm on May 3, 2020: member

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

    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.

  7. 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:

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

    and become:

    0SET_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 0:15 am on May 5, 2020:
    Yeah didn’t check if all follow the same pattern. Separate PR is fine.
  8. promag commented at 11:25 pm on May 3, 2020: member
    Concept ACK.
  9. DrahtBot added the label Needs rebase on May 5, 2020
  10. fanquake added the label Build system on May 5, 2020
  11. 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.

  12. vasild force-pushed on May 5, 2020
  13. DrahtBot removed the label Needs rebase on May 5, 2020
  14. DrahtBot added the label Needs rebase on May 6, 2020
  15. vasild force-pushed on May 6, 2020
  16. DrahtBot removed the label Needs rebase on May 6, 2020
  17. DrahtBot added the label Needs rebase on May 11, 2020
  18. vasild force-pushed on May 11, 2020
  19. vasild commented at 9:22 am on May 11, 2020: member
    Rebased.
  20. DrahtBot removed the label Needs rebase on May 11, 2020
  21. DrahtBot added the label Needs rebase on May 13, 2020
  22. vasild force-pushed on May 13, 2020
  23. vasild commented at 12:32 pm on May 13, 2020: member
    Rebased.
  24. DrahtBot removed the label Needs rebase on May 13, 2020
  25. DrahtBot added the label Needs rebase on May 13, 2020
  26. vasild force-pushed on May 21, 2020
  27. vasild commented at 8:15 am on May 21, 2020: member
    Rebased.
  28. DrahtBot removed the label Needs rebase on May 21, 2020
  29. DrahtBot added the label Needs rebase on May 28, 2020
  30. vasild force-pushed on May 28, 2020
  31. vasild commented at 8:00 pm on May 28, 2020: member
    Rebased.
  32. DrahtBot removed the label Needs rebase on May 28, 2020
  33. vasild force-pushed on Jun 4, 2020
  34. vasild commented at 10:49 am on June 4, 2020: member
    Rebased.
  35. DrahtBot added the label Needs rebase on Jun 4, 2020
  36. vasild force-pushed on Jun 4, 2020
  37. vasild commented at 7:26 pm on June 4, 2020: member
    Rebased.
  38. DrahtBot removed the label Needs rebase on Jun 4, 2020
  39. DrahtBot added the label Needs rebase on Jun 8, 2020
  40. vasild force-pushed on Jun 8, 2020
  41. vasild commented at 7:32 pm on June 8, 2020: member
    Rebased.
  42. DrahtBot removed the label Needs rebase on Jun 8, 2020
  43. DrahtBot added the label Needs rebase on Jul 15, 2020
  44. build: avoid repetitions when enabling warnings 
    Introduce a macro APPEND_FLAG_IF_AVAILABLE() and use that.
    2a7275827a
  45. vasild force-pushed on Jul 16, 2020
  46. vasild commented at 8:53 am on July 16, 2020: member
    Rebased due to conflicts. @fanquake, what’s your take on this, now that #18882 has been merged?
  47. DrahtBot removed the label Needs rebase on Jul 16, 2020
  48. 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.

  49. DrahtBot added the label Needs rebase on Aug 18, 2020
  50. DrahtBot commented at 4:36 am on August 18, 2020: member

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

    Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

  51. vasild commented at 7:03 am on August 18, 2020: member

    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.

  52. vasild closed this on Aug 18, 2020
  53. 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.

  54. DrahtBot locked this on Feb 15, 2022


vasild MarcoFalke brakmic practicalswift DrahtBot promag fanquake hebasto

Labels
Refactoring Build system Needs rebase

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-11-17 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me