Enable -Wstring-concatenation and -Wstring-conversion on clang builds #26288

Empact · 2022-10-10T20:26:28Z

The only current violations are a few uses of assert(!"descriptive string").

Seems the benefit of ensuring against a code fault outweighs the benefit of possible additional information in these cases.

-Wstring-concatenation
Diagnostic text:

warning: suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma?

-Wstring-conversion
Diagnostic text:

warning: implicit conversion turns string literal into bool: A to B

https://clang.llvm.org/docs/DiagnosticsReference.html#wstring-concatenation

DrahtBot · 2022-10-10T23:52:28Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	theuni

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28191 (refactor: Remove unused MessageStartChars parameters from BlockManager methods by MarcoFalke)
#26326 (net: don't lock cs_main while reading blocks in net processing by andrewtoth)
#25972 (build: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set by fanquake)

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.

theuni

Concept ACK. This has been open for a while, I'm curious to know if any new violations have snuck in since?

src/net_processing.cpp

src/script/interpreter.cpp

hebasto · 2023-05-04T14:53:26Z

Concept ACK. This has been open for a while, I'm curious to know if any new violations have snuck in since?

Maybe rebase it?

Empact · 2023-06-29T15:31:06Z

Rebased. 👍

theuni · 2023-06-29T18:28:51Z

Thanks! Now mind updating to use Assert()? :)

The only current violations are a few uses of assert(!"descriptive string"). Seems the benefit of ensuring against a code fault outweighs the benefit of possible additional information in these cases.

…rom util/check

Empact · 2023-07-17T21:11:37Z

Rebased, converted assignments to initialization, and moved to Assert where possible. I could not apply Assert(false) in interpreter.cpp due to errors, but #26504 could be applied here once merged.

DrahtBot · 2023-08-07T10:21:03Z

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

maflcko · 2023-09-20T16:19:05Z

src/script/interpreter.cpp

@@ -934,7 +934,7 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
                    case OP_ABS:        if (bn < bnZero) bn = -bn; break;
                    case OP_NOT:        bn = (bn == bnZero); break;
                    case OP_0NOTEQUAL:  bn = (bn != bnZero); break;
-                    default:            assert(!"invalid opcode"); break;
+                    default:            assert(false); break; // invalid opcode


Not sure what the goal here is? This will print something else on stderr when the assert is hit.

Pretty sure the previous code is commonly used and understood?

maflcko · 2023-09-20T16:21:14Z

Closing for now due to lack of progress. Let us know if this should be reopened.

Empact changed the title ~~Enable -Wstrinng-concatenation and -Wstring-conversion on clang builds~~ Enable -Wstring-concatenation and -Wstring-conversion on clang builds Oct 10, 2022

Empact force-pushed the 2022-10-warn-string branch 2 times, most recently from 3a127d9 to 6709e7e Compare October 10, 2022 20:41

DrahtBot mentioned this pull request Oct 11, 2022

build: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set #25972

Merged

Empact force-pushed the 2022-10-warn-string branch from 6709e7e to dc37ef3 Compare October 11, 2022 05:46

maflcko mentioned this pull request Oct 17, 2022

net: don't lock cs_main while reading blocks in net processing #26326

Merged

This was referenced Oct 17, 2022

use shared mutex to guard against block files being removed before read #26316

Closed

Drop IO priority to idle while reading blocks for peer requests and startup verification #9245

Closed

DrahtBot mentioned this pull request Oct 28, 2022

rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block #26415

Merged

This was referenced Nov 16, 2022

Add UNREACHABLE macro #26504

Closed

refactor: Extract MIB_BYTES constant for init.cpp #25386

Closed

DrahtBot mentioned this pull request Feb 19, 2023

refactor, kernel: Decouple ArgsManager from blockstorage #27125

Merged

achow101 requested review from hebasto and theuni April 25, 2023 15:18

theuni suggested changes May 4, 2023

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

src/net_processing.cpp Outdated Show resolved Hide resolved

src/script/interpreter.cpp Show resolved Hide resolved

DrahtBot mentioned this pull request May 5, 2023

kernel: Remove args, settings, chainparams, chainparamsbase from kernel library #27576

Merged

Empact force-pushed the 2022-10-warn-string branch from dc37ef3 to 697fa4d Compare May 5, 2023 19:38

DrahtBot added the Needs rebase label May 11, 2023

Empact force-pushed the 2022-10-warn-string branch 2 times, most recently from f695a79 to 2d6afa4 Compare June 28, 2023 21:42

DrahtBot added the CI failed label Jun 28, 2023

Empact force-pushed the 2022-10-warn-string branch from 2d6afa4 to f26f19f Compare June 28, 2023 21:55

DrahtBot removed Needs rebase CI failed labels Jun 28, 2023

Empact force-pushed the 2022-10-warn-string branch from f26f19f to 2c0e21c Compare July 17, 2023 20:19

Enable -Wstring-concatenation and -Wstring-conversion on clang builds

1075262

The only current violations are a few uses of assert(!"descriptive string"). Seems the benefit of ensuring against a code fault outweighs the benefit of possible additional information in these cases.

Empact force-pushed the 2022-10-warn-string branch from 2c0e21c to d928fb5 Compare July 17, 2023 20:22

DrahtBot added the CI failed label Jul 17, 2023

Empact force-pushed the 2022-10-warn-string branch from d928fb5 to 06f5558 Compare July 17, 2023 20:32

refactor: Convert string-conversion violation asserts to use Assert f…

50e7f52

…rom util/check

Empact force-pushed the 2022-10-warn-string branch from 06f5558 to 50e7f52 Compare July 17, 2023 20:39

fanquake mentioned this pull request Jul 18, 2023

failure in wallet_resendwallettransactions.py --legacy-wallet #28094

Closed

DrahtBot mentioned this pull request Aug 1, 2023

refactor: Remove unused MessageStartChars parameters from BlockManager methods #28191

Merged

DrahtBot added the Needs rebase label Aug 7, 2023

maflcko reviewed Sep 20, 2023

View reviewed changes

maflcko closed this Sep 20, 2023

bitcoin locked and limited conversation to collaborators Sep 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Enable -Wstring-concatenation and -Wstring-conversion on clang builds #26288

Enable -Wstring-concatenation and -Wstring-conversion on clang builds #26288

Uh oh!

Empact commented Oct 10, 2022 •

edited

Loading

Uh oh!

DrahtBot commented Oct 10, 2022 •

edited

Loading

Uh oh!

theuni left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hebasto commented May 4, 2023

Uh oh!

Empact commented Jun 29, 2023

Uh oh!

theuni commented Jun 29, 2023

Uh oh!

Empact commented Jul 17, 2023

Uh oh!

DrahtBot commented Aug 7, 2023

Uh oh!

maflcko Sep 20, 2023

Uh oh!

maflcko commented Sep 20, 2023

Uh oh!

Uh oh!

Enable -Wstring-concatenation and -Wstring-conversion on clang builds #26288

Enable -Wstring-concatenation and -Wstring-conversion on clang builds #26288

Uh oh!

Conversation

Empact commented Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

theuni left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

hebasto commented May 4, 2023

Uh oh!

Empact commented Jun 29, 2023

Uh oh!

theuni commented Jun 29, 2023

Uh oh!

Empact commented Jul 17, 2023

Uh oh!

DrahtBot commented Aug 7, 2023

Uh oh!

maflcko Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko commented Sep 20, 2023

Uh oh!

Uh oh!

Empact commented Oct 10, 2022 •

edited

Loading

DrahtBot commented Oct 10, 2022 •

edited

Loading

theuni left a comment •

edited

Loading