msvc, refactor: Avoid some rare compiler warnings #25819

1. 
   hebasto commented at 0: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

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

   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. 
   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.

6. hebasto removed the label Windows on Aug 11, 2022
7. 
   hebasto commented at 6:52 pm on August 11, 2022: member

   I can tell they’re not Windows specific.

   True.

8. 
   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.
9. 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.
10. 
    Zhaojun-Liu commented at 2:31 am on September 26, 2022: none

    Any updates of this PR? @hebasto @MarcoFalke Thank you~
11. 
    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.

12. fanquake referenced this in commit 1730f6cb23 on Oct 3, 2022
13. DrahtBot added the label Needs rebase on Oct 3, 2022
14. sidhujag referenced this in commit 861db05f8b on Oct 4, 2022
15. msvc, refactor: Avoid compiler warning C4018

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

    cf3686eacf
16. msvc, refactor: Avoid compiler warning C4334

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

    942efe7b2f
17. hebasto force-pushed on Oct 4, 2022
18. DrahtBot removed the label Needs rebase on Oct 4, 2022
19. 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?
20. 
    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.

21. hebasto closed this on Oct 4, 2022

    ---

22. bitcoin locked this on Oct 4, 2023

Contributors
hebasto DrahtBot sipsorcery Zhaojun-Liu MarcoFalke

Review Requested
sipsorcery