Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: split warnings out of CXXFLAGS #13306

Merged
merged 1 commit into from
May 28, 2018
Merged

Conversation

theuni
Copy link
Member

@theuni theuni commented May 22, 2018

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.

CXXFLAGS should not be modified anyway. Also, this will enable us to
selectively disable warnings.
@Empact
Copy link
Contributor

Empact commented May 22, 2018

Concept ack. Could be done with a single var WARN_CXXFLAGS.

@theuni
Copy link
Member Author

theuni commented May 22, 2018

@Empact They were split so that we may filter out warnings, but still quash annoying ones.

@Empact
Copy link
Contributor

Empact commented May 22, 2018

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
Copy link
Contributor

Very good idea! Will allow for enabling more warnings

Concept ACK

@laanwj
Copy link
Member

laanwj commented May 23, 2018

utACK 9e305b5

@sipa
Copy link
Member

sipa commented May 24, 2018

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

@laanwj
Copy link
Member

laanwj commented May 24, 2018

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
Copy link
Member Author

theuni commented May 24, 2018

@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
Copy link
Member

utACK 9e305b5

@laanwj laanwj merged commit 9e305b5 into bitcoin:master May 28, 2018
laanwj added a commit that referenced this pull request May 28, 2018
9e305b5 build: split warnings out of CXXFLAGS (Cory Fields)

Pull request description:

  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.

Tree-SHA512: 1bf686250f7a59c0aff04371f87c5db4e8f5bde604c6ab75e568326fb6d7733f26b113fa52dc1c836fa10baa76770d479a0e5f82a4a1905947dd7f245e0560f4
kwvg added a commit to kwvg/dash that referenced this pull request Jun 17, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Jun 17, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Jun 17, 2021
TheArbitrator pushed a commit to TheArbitrator/dash that referenced this pull request Jun 21, 2021
9e305b5 build: split warnings out of CXXFLAGS (Cory Fields)

Pull request description:

  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.

Tree-SHA512: 1bf686250f7a59c0aff04371f87c5db4e8f5bde604c6ab75e568326fb6d7733f26b113fa52dc1c836fa10baa76770d479a0e5f82a4a1905947dd7f245e0560f4
UdjinM6 added a commit to dashpay/dash that referenced this pull request Jun 23, 2021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants