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

vasild · 2020-05-03T14:19:21Z

Introduce a macro APPEND_FLAG_IF_AVAILABLE() and use that.

maflcko · 2020-05-03T14:26:30Z

Concept ACK b26f8a0

brakmic · 2020-05-03T14:57:06Z

Concept ACK b26f8a0

practicalswift · 2020-05-03T19:29:30Z

Concept ACK

Very nice initiative: this repetition has annoyed me :)

DrahtBot · 2020-05-03T23:06:13Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

build: Enable some commonly enabled compiler diagnostics #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.

promag

Concept ACK.

promag · 2020-05-03T23:23:21Z

configure.ac

@@ -333,31 +333,36 @@ if test x$use_sanitizers != x; then
    ]],[[]])])
 fi

+dnl APPEND_FLAG_IF_AVAILABLE([APPEND_TO_THIS_VARIABLE], [-WallExtra])
+AC_DEFUN([APPEND_FLAG_IF_AVAILABLE], [


Could also define SET_FLAG_IF_AVAILABLE and use throughout.

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.

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

fanquake · 2020-05-05T07:35:23Z

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.

vasild · 2020-05-11T09:22:14Z

Rebased.

vasild · 2020-05-13T12:32:36Z

Rebased.

vasild · 2020-05-28T20:00:55Z

Rebased.

vasild · 2020-06-04T10:49:26Z

Rebased.

vasild · 2020-06-04T19:26:39Z

Rebased.

vasild · 2020-06-08T19:32:54Z

Rebased.

Introduce a macro APPEND_FLAG_IF_AVAILABLE() and use that.

vasild · 2020-07-16T08:53:14Z

Rebased due to conflicts.

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

fanquake · 2020-08-10T10:24:15Z

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

DrahtBot · 2020-08-18T04:36:41Z

🐙 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".}

vasild · 2020-08-18T07:03:45Z

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.

hebasto · 2020-08-18T07:11:58Z

I tend to agree with @fanquake:

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

maflcko added the Refactoring label May 3, 2020

vasild mentioned this pull request May 3, 2020

build: warn on potentially uninitialized reads #18843

Merged

promag reviewed May 3, 2020

View reviewed changes

This was referenced May 4, 2020

test, build: Enable -Werror=sign-compare #18216

Merged

build: ensure we aren't using GNU extensions #18088

Merged

build: Enable -Wsuggest-override if available #16710

Merged

DrahtBot added the Needs rebase label May 5, 2020

fanquake added the Build system label May 5, 2020

vasild force-pushed the avoid_repetitions_in_configure.ac branch from b26f8a0 to f14b215 Compare May 5, 2020 11:42

DrahtBot removed the Needs rebase label May 5, 2020

This was referenced May 5, 2020

build: enable -Werror=gnu #18887

Merged

build: fix -Wformat-security check when compiling with GCC #18882

Merged

DrahtBot added the Needs rebase label May 6, 2020

vasild force-pushed the avoid_repetitions_in_configure.ac branch from f14b215 to d49d213 Compare May 6, 2020 14:15

DrahtBot removed the Needs rebase label May 6, 2020

DrahtBot mentioned this pull request May 9, 2020

Replace -Wthread-safety-analysis with broader -Wthread-safety #18635

Merged

DrahtBot added the Needs rebase label May 11, 2020

vasild force-pushed the avoid_repetitions_in_configure.ac branch from d49d213 to e5d3519 Compare May 11, 2020 09:19

DrahtBot added Needs rebase and removed Needs rebase labels May 11, 2020

vasild force-pushed the avoid_repetitions_in_configure.ac branch from e5d3519 to c635ea8 Compare May 13, 2020 12:31

DrahtBot added Needs rebase and removed Needs rebase labels May 13, 2020

DrahtBot mentioned this pull request May 22, 2020

build: Enable some commonly enabled compiler diagnostics #19015

Merged

DrahtBot added the Needs rebase label May 28, 2020

vasild force-pushed the avoid_repetitions_in_configure.ac branch from cecdf95 to dea032a Compare May 28, 2020 20:00

DrahtBot removed the Needs rebase label May 28, 2020

DrahtBot mentioned this pull request Jun 2, 2020

refactor: Fix unreachable code in init arg checks #19131

Merged

vasild force-pushed the avoid_repetitions_in_configure.ac branch from dea032a to e6e1a0c Compare June 4, 2020 10:49

DrahtBot added the Needs rebase label Jun 4, 2020

vasild force-pushed the avoid_repetitions_in_configure.ac branch from e6e1a0c to 86a0367 Compare June 4, 2020 19:26

DrahtBot removed the Needs rebase label Jun 4, 2020

DrahtBot mentioned this pull request Jun 6, 2020

test: Avoid overwriting the NodeContext member of the testing setup [-Wshadow-field] #19188

Merged

DrahtBot added the Needs rebase label Jun 8, 2020

vasild force-pushed the avoid_repetitions_in_configure.ac branch from 86a0367 to 7b72014 Compare June 8, 2020 19:31

DrahtBot removed the Needs rebase label Jun 8, 2020

DrahtBot added the Needs rebase label Jul 15, 2020

build: avoid repetitions when enabling warnings

2a72758

Introduce a macro APPEND_FLAG_IF_AVAILABLE() and use that.

vasild force-pushed the avoid_repetitions_in_configure.ac branch from 7b72014 to 2a72758 Compare July 16, 2020 08:51

DrahtBot removed the Needs rebase label Jul 16, 2020

DrahtBot added the Needs rebase label Aug 18, 2020

vasild closed this Aug 18, 2020

fanquake mentioned this pull request Mar 18, 2021

build: Add convenient BITCOIN_TRY_ADD_COMPILE_FLAG macro #21459

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

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

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

Uh oh!

Conversation

vasild commented May 3, 2020

Uh oh!

maflcko commented May 3, 2020

Uh oh!

brakmic commented May 3, 2020

Uh oh!

practicalswift commented May 3, 2020

Uh oh!

DrahtBot commented May 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag May 3, 2020

Choose a reason for hiding this comment

Uh oh!

vasild May 4, 2020

Choose a reason for hiding this comment

Uh oh!

promag May 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fanquake commented May 5, 2020

Uh oh!

vasild commented May 11, 2020

Uh oh!

vasild commented May 13, 2020

Uh oh!

vasild commented May 28, 2020

Uh oh!

vasild commented Jun 4, 2020

Uh oh!

vasild commented Jun 4, 2020

Uh oh!

vasild commented Jun 8, 2020

Uh oh!

vasild commented Jul 16, 2020

Uh oh!

fanquake commented Aug 10, 2020

Uh oh!

DrahtBot commented Aug 18, 2020

Uh oh!

vasild commented Aug 18, 2020

Uh oh!

hebasto commented Aug 18, 2020

Uh oh!

Uh oh!

DrahtBot commented May 3, 2020 •

edited

Loading

promag May 5, 2020 •

edited

Loading