build: split warnings out of CXXFLAGS #13306

pull theuni wants to merge 1 commits into bitcoin:master from theuni:debug_flags changing 2 files +15 −13

theuni commented at 10:18 PM on May 22, 2018: member

CXXFLAGS should not be modified anyway. Also, this will enable us to selectively disable warnings.

As discussed with @sipa on IRC. Intention is to be able to filter out warnings from leveldb code so that we can be more aggressive with what we enable.

build: split warnings out of CXXFLAGS

CXXFLAGS should not be modified anyway. Also, this will enable us to
selectively disable warnings.

9e305b56f5

MarcoFalke added the label Build system on May 22, 2018
Empact commented at 10:33 PM on May 22, 2018: member

Concept ack. Could be done with a single var WARN_CXXFLAGS.
theuni commented at 10:37 PM on May 22, 2018: member

@Empact They were split so that we may filter out warnings, but still quash annoying ones.
Empact commented at 10:39 PM on May 22, 2018: member

To be clear, I just mean that you could store both the warn and nowarn options in the WARN_CXXFLAGS. No change in options communicated.
practicalswift commented at 2:36 PM on May 23, 2018: contributor

Very good idea! Will allow for enabling more warnings

Concept ACK
laanwj commented at 6:18 PM on May 23, 2018: member

utACK 9e305b56f5323cbf2b793d96e63e1a1c107155ab
sipa commented at 12:39 AM on May 24, 2018: member

Concept ACK. I don't understand autotools enough to see the effect of these code changes.
laanwj commented at 9:31 PM on May 24, 2018: member

Concept ACK. I don't understand autotools enough to see the effect of these code changes.

As I understand it, there isn't any effect as-is, this just isolates the warnings into a different variable.
theuni commented at 9:45 PM on May 24, 2018: member
@Empact That would not change the result here, but it would mean that we're unable to make a distinction between the two later. @laanwj yes, exactly. @sipa mentioned that he would like to add more aggressive warnings for our code (-Wsuggest-override specifically was the prompt), but that would mean that we would have to fix up a bunch of warnings inside of leveldb. This PR separates the warnings so that they can be removed from leveldb as such:
```
diff --git a/src/Makefile.leveldb.include b/src/Makefile.leveldb.include
index 833f3d2..36edef9 100644
--- a/src/Makefile.leveldb.include
+++ b/src/Makefile.leveldb.include
@@ -30,7 +30,7 @@ LEVELDB_CPPFLAGS_INT += -DLEVELDB_PLATFORM_POSIX
 endif

 leveldb_libleveldb_a_CPPFLAGS = $(AM_CPPFLAGS) $(LEVELDB_CPPFLAGS_INT) $(LEVELDB_CPPFLAGS)
-leveldb_libleveldb_a_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
+leveldb_libleveldb_a_CXXFLAGS = $(filter-out $(WARN_CXXFLAGS),$(AM_CXXFLAGS)) $(PIE_FLAGS)

 leveldb_libleveldb_a_SOURCES=
 leveldb_libleveldb_a_SOURCES += leveldb/port/atomic_pointer.h
```
I didn't include that in this PR because I don't think we want to remove all warnings, so that needs discussion, but this PR is an obvious and straightforward prerequisite IMO.
fanquake commented at 1:07 PM on May 28, 2018: member

utACK 9e305b5
laanwj merged this on May 28, 2018
laanwj closed this on May 28, 2018
laanwj referenced this in commit f5a7733ff7 on May 28, 2018
kittywhiskers referenced this in commit ab5ae221f1 on Jun 17, 2021
kittywhiskers referenced this in commit 64569fbd2f on Jun 17, 2021
kittywhiskers referenced this in commit 29288dc858 on Jun 17, 2021
TheArbitrator referenced this in commit 928be77d6a on Jun 21, 2021
UdjinM6 referenced this in commit 988fa6a235 on Jun 23, 2021
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels