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

1. 
   Empact commented at 8:26 PM on October 10, 2022: member

   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

2. Empact renamed this:
   Enable -Wstrinng-concatenation and -Wstring-conversion on clang builds
   Enable -Wstring-concatenation and -Wstring-conversion on clang builds
   on Oct 10, 2022
3. Empact force-pushed on Oct 10, 2022
4. Empact force-pushed on Oct 10, 2022
5. 
   DrahtBot commented at 11:52 PM on October 10, 2022: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--021abf342d371248e50ceaed478a90ca-->

   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.

   <!--174a7506f384e20aa4161008e828411d-->

   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.

6. Empact force-pushed on Oct 11, 2022
7. achow101 requested review from hebasto on Apr 25, 2023
8. achow101 requested review from theuni on Apr 25, 2023
9. in src/net_processing.cpp:2145 in dc37ef38a5 outdated

   2140 | @@ -2141,17 +2141,15 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv&
   2141 |          // Fast-path: in this case it is possible to serve the block directly from disk,
   2142 |          // as the network format matches the format on disk
   2143 |          std::vector<uint8_t> block_data;
   2144 | -        if (!ReadRawBlockFromDisk(block_data, pindex->GetBlockPos(), m_chainparams.MessageStart())) {
   2145 | -            assert(!"cannot load block from disk");
   2146 | -        }
   2147 | +        // fail if cannot load block from disk
   2148 | +        assert(ReadRawBlockFromDisk(block_data, pindex->GetBlockPos(), m_chainparams.MessageStart()));
   

   ---

   ---

   

   theuni commented at 2:31 PM on May 4, 2023:

   Could you please split this up like this instead?

   auto foo = ReadRawBlockFromDisk(...);
   assert(foo);
   

   That way the we don't introduce a new side-effect of NDEBUG=1

   ---

   

   Empact commented at 3:30 PM on June 29, 2023:

   Done

10. in src/net_processing.cpp:2152 in dc37ef38a5 outdated

    2153 |          std::shared_ptr<CBlock> pblockRead = std::make_shared<CBlock>();
    2154 | -        if (!ReadBlockFromDisk(*pblockRead, pindex, m_chainparams.GetConsensus())) {
    2155 | -            assert(!"cannot load block from disk");
    2156 | -        }
    2157 | +        // fail if cannot load block from disk
    2158 | +        assert(ReadBlockFromDisk(*pblockRead, pindex, m_chainparams.GetConsensus()));
    

    ---

    ---

    

    theuni commented at 2:31 PM on May 4, 2023:

    Likewise here.

    ---

    

    Empact commented at 3:30 PM on June 29, 2023:

    Done

11. in src/script/interpreter.cpp:1467 in dc37ef38a5 outdated

    1463 | @@ -1464,14 +1464,11 @@ const HashWriter HASHER_TAPBRANCH{TaggedHash("TapBranch")};
    1464 |  
    1465 |  static bool HandleMissingData(MissingDataBehavior mdb)
    1466 |  {
    1467 | -    switch (mdb) {
    

    ---

    ---

    

    theuni commented at 2:36 PM on May 4, 2023:

    Imo it'd be cleaner and more readable to just leave the logic here as before, turning the wonky string assertions into comments.

    ---

    

    Empact commented at 3:30 PM on June 29, 2023:

    Done

12. theuni changes_requested
13. 
    theuni commented at 2:37 PM on May 4, 2023: member

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

14. 
    hebasto commented at 2:53 PM on May 4, 2023: member

    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?

15. Empact force-pushed on May 5, 2023
16. DrahtBot added the label Needs rebase on May 11, 2023
17. Empact force-pushed on Jun 28, 2023
18. Empact force-pushed on Jun 28, 2023
19. DrahtBot added the label CI failed on Jun 28, 2023
20. Empact force-pushed on Jun 28, 2023
21. DrahtBot removed the label Needs rebase on Jun 28, 2023
22. DrahtBot removed the label CI failed on Jun 29, 2023
23. 
    Empact commented at 3:31 PM on June 29, 2023: member

    Rebased. 👍

24. 
    theuni commented at 6:28 PM on June 29, 2023: member

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

25. Empact force-pushed on Jul 17, 2023
26. Enable -Wstring-concatenation and -Wstring-conversion on clang builds

    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.

    10752620db
27. Empact force-pushed on Jul 17, 2023
28. DrahtBot added the label CI failed on Jul 17, 2023
29. Empact force-pushed on Jul 17, 2023
30. refactor: Convert string-conversion violation asserts to use Assert from util/check 50e7f5284b
31. Empact force-pushed on Jul 17, 2023
32. 
    Empact commented at 9:11 PM on July 17, 2023: member

    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.

33. DrahtBot added the label Needs rebase on Aug 7, 2023
34. 
    DrahtBot commented at 10:21 AM on August 7, 2023: contributor

    <!--cf906140f33d8803c4a75a2196329ecb-->

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

35. in src/script/interpreter.cpp:937 in 50e7f5284b

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

    ---

    ---

    

    maflcko commented at 4:19 PM on September 20, 2023:

    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?

36. 
    maflcko commented at 4:21 PM on September 20, 2023: member

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

37. maflcko closed this on Sep 20, 2023

    ---

38. bitcoin locked this on Sep 19, 2024

Contributors

EmpactDrahtBottheunihebastomaflcko

Review Requested
hebasto