msvc, refactor: Avoid some rare compiler warnings #25819

1. 
   hebasto commented at 12:09 AM on August 11, 2022: member

   This PR:

   • removes 3 warnings from the global DisableSpecificWarnings list
   • improves code hygiene
2. hebasto added the label Windows on Aug 11, 2022
3. 
   DrahtBot commented at 3:26 AM on August 11, 2022: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #26045 (rpc: Optimize serialization disk space of dumptxoutset by aureleoules)
   • #25081 (tracing: lock contention analysis by martinus)
   • #22338 ([Refactor]: Rename Script methods that only work on PreTapScript scripts by sanket1729)

   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.

4. fanquake requested review from sipsorcery on Aug 11, 2022
5. DrahtBot cross-referenced this on Aug 11, 2022 from issue tracing: lock contention analysis by martinus
6. DrahtBot cross-referenced this on Aug 11, 2022 from issue [Refactor]: Rename Script methods that only work on PreTapScript scripts by sanket1729
7. 
   sipsorcery commented at 6:38 PM on August 11, 2022: member

   Wouldn't this be better in two separate PR's?

   I realise the code changes are trivial but as far as I can tell they're not Windows specific. It wouldn't be the first time an innocuos cast has had a side effect. IMHO the warnings removal and code changes should be in separate PR's.

8. hebasto removed the label Windows on Aug 11, 2022
9. 
   hebasto commented at 6:52 PM on August 11, 2022: member

   I can tell they're not Windows specific.

   True.

10. hebasto cross-referenced this on Sep 6, 2022 from issue [MSVC] Bitcoin failed to build with waring c4858 on MSVC by fangzhouxia
11. DrahtBot cross-referenced this on Sep 8, 2022 from issue rpc: Optimize serialization disk space of dumptxoutset by aureleoules
12. 
    Zhaojun-Liu commented at 2:35 AM on September 20, 2022: none

    Hi @hebasto, We update the bitcoin source recently, and encountered the waring C4858, and found that this issue (https://github.com/bitcoin/bitcoin/issues/26017) has been fixed in commit 31168dd, could this PR be merged as soon as possible? Thanks.

13. in src/bech32.cpp:244 in 10cc8b151f outdated

     240 | @@ -241,7 +241,7 @@ constexpr std::array<uint32_t, 25> GenerateSyndromeConstants() {
     241 |      std::array<uint32_t, 25> SYNDROME_CONSTS{};
     242 |      for (int k = 1; k < 6; ++k) {
     243 |          for (int shift = 0; shift < 5; ++shift) {
     244 | -            int16_t b = GF1024_LOG.at(1 << shift);
     245 | +            int16_t b = GF1024_LOG.at(1ULL << shift);
    

    ---

    ---

    

    MarcoFalke commented at 8:16 AM on September 20, 2022:

    It seems msvc violates the c++ standard by incorrectly promoting this to 64 bit

    ---

    

    hebasto commented at 6:36 PM on October 4, 2022:

    The result of the shift operation is promoted to size_t as std::array::at() expects an argument of such a type.

14. 
    Zhaojun-Liu commented at 2:31 AM on September 26, 2022: none

    Any updates of this PR? @hebasto @MarcoFalke Thank you~

15. hebasto cross-referenced this on Sep 27, 2022 from issue refactor: Do not discard `try_lock()` return value by hebasto
16. 
    hebasto commented at 9:36 PM on September 27, 2022: member

    Wouldn't this be better in two separate PR's?

    I realise the code changes are trivial but as far as I can tell they're not Windows specific. It wouldn't be the first time an innocuos cast has had a side effect. IMHO the warnings removal and code changes should be in separate PR's.

    I separated some changes into #26189.

17. fanquake referenced this in commit 1730f6cb23 on Oct 3, 2022
18. DrahtBot added the label Needs rebase on Oct 3, 2022
19. sidhujag referenced this in commit 861db05f8b on Oct 4, 2022
20. msvc, refactor: Avoid compiler warning C4018

    https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4018

    cf3686eacf
21. msvc, refactor: Avoid compiler warning C4334

    https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4334

    942efe7b2f
22. hebasto force-pushed on Oct 4, 2022
23. DrahtBot removed the label Needs rebase on Oct 4, 2022
24. in src/script/script.h:212 in 942efe7b2f

     208 | @@ -209,7 +209,7 @@ enum opcodetype
     209 |  };
     210 |  
     211 |  // Maximum value that an opcode can be
     212 | -static const unsigned int MAX_OPCODE = OP_NOP10;
     213 | +static constexpr auto MAX_OPCODE = OP_NOP10;
    

    ---

    ---

    

    MarcoFalke commented at 12:12 PM on October 4, 2022:

    If this is changed, it wouldn't be less effort to review making the underlying type explicit? Something like #22232 ?

    ---

    

    hebasto commented at 2:51 PM on October 4, 2022:

    In addition, isn't clearer to move MAX_OPCODE = OP_NOP10 into enum opcodetype definition?

25. DrahtBot cross-referenced this on Oct 4, 2022 from issue build, msvc: Enable C4834 warning by hebasto
26. 
    hebasto commented at 7:15 PM on October 4, 2022: member

    Changes in src/bech32.cpp has been split into #26252.

    Touching consensus code for the sake of a MSVC warning does not look worthy, although making an enum opcodetype underlying type explicit is a good idea.

    So closing this PR.

27. hebasto closed this on Oct 4, 2022

    ---

28. bitcoin locked this on Oct 4, 2023

Contributors

hebastoDrahtBotsipsorceryZhaojun-LiuMarcoFalke

Review Requested
sipsorcery