Skip to content

msvc, refactor: Avoid some rare compiler warnings #25819

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

msvc, refactor: Avoid some rare compiler warnings #25819

wants to merge 2 commits into from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Aug 11, 2022

This PR:

  • removes 3 warnings from the global DisableSpecificWarnings list
  • improves code hygiene

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 11, 2022 •
edited
Loading

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.

@fanquake fanquake requested a review from sipsorcery August 11, 2022 13:25
This was referenced Aug 11, 2022
Closed
Closed
@sipsorcery
Copy link
Contributor

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.

@hebasto hebasto removed the Windows label Aug 11, 2022
@hebasto
Copy link
Member Author

hebasto commented Aug 11, 2022

I can tell they're not Windows specific.

True.

@hebasto hebasto mentioned this pull request Sep 6, 2022
Closed
@DrahtBot DrahtBot mentioned this pull request Sep 8, 2022
Closed
@Zhaojun-Liu
Copy link

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

maflcko
maflcko reviewed Sep 20, 2022
View reviewed changes
src/bech32.cpp
@@ -241,7 +241,7 @@ constexpr std::array<uint32_t, 25> GenerateSyndromeConstants() {
std::array<uint32_t, 25> SYNDROME_CONSTS{};
for (int k = 1; k < 6; ++k) {
for (int shift = 0; shift < 5; ++shift) {
int16_t b = GF1024_LOG.at(1 << shift);
int16_t b = GF1024_LOG.at(1ULL << shift);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@hebasto hebasto Oct 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Zhaojun-Liu
Copy link

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

@hebasto hebasto mentioned this pull request Sep 27, 2022
Merged
@hebasto
Copy link
Member Author

hebasto commented Sep 27, 2022

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.

fanquake added a commit to bitcoin-core/gui that referenced this pull request Oct 3, 2022
@fanquake
Merge bitcoin/bitcoin#26189: refactor: Do not discard try_lock() re…
1730f6c 
…turn value

30cc1c6 refactor: Drop `owns_lock()` call (Hennadii Stepanov)
bff4e06 refactor: Do not discard `try_lock()` return value (Hennadii Stepanov)

Pull request description:

  Microsoft's C++ Standard Library uses the `[[nodiscard]]` attribute for `try_lock()`.
  See: https://github.com/microsoft/STL/blob/main/stl/inc/mutex

  This change allows to drop the current suppression for the warning C4838 and helps to prevent the upcoming warning C4858.
  See: microsoft/STL@539c26c

  Fixes bitcoin/bitcoin#26017.

  Split from bitcoin/bitcoin#25819.

ACKs for top commit:
  vasild:
    ACK 30cc1c6

Tree-SHA512: ce17404e1c78af4f763129753caf8e5a0e1c91ba398778fe912f9fcc56a847e8112460d1a1a35bf905a593b7d8e0b16c6b099ad74976b67dca5f4f3eda6ff621
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 4, 2022
@fanquake
Merge bitcoin#26189: refactor: Do not discard try_lock() return value
861db05 
30cc1c6 refactor: Drop `owns_lock()` call (Hennadii Stepanov)
bff4e06 refactor: Do not discard `try_lock()` return value (Hennadii Stepanov)

Pull request description:

  Microsoft's C++ Standard Library uses the `[[nodiscard]]` attribute for `try_lock()`.
  See: https://github.com/microsoft/STL/blob/main/stl/inc/mutex

  This change allows to drop the current suppression for the warning C4838 and helps to prevent the upcoming warning C4858.
  See: microsoft/STL@539c26c

  Fixes bitcoin#26017.

  Split from bitcoin#25819.

ACKs for top commit:
  vasild:
    ACK 30cc1c6

Tree-SHA512: ce17404e1c78af4f763129753caf8e5a0e1c91ba398778fe912f9fcc56a847e8112460d1a1a35bf905a593b7d8e0b16c6b099ad74976b67dca5f4f3eda6ff621
maflcko
maflcko reviewed Oct 4, 2022
View reviewed changes
src/script/script.h
@@ -209,7 +209,7 @@ enum opcodetype
};

// Maximum value that an opcode can be
static const unsigned int MAX_OPCODE = OP_NOP10;
static constexpr auto MAX_OPCODE = OP_NOP10;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@DrahtBot DrahtBot mentioned this pull request Oct 4, 2022
Merged
@hebasto
Copy link
Member Author

hebasto commented Oct 4, 2022

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.

@hebasto hebasto closed this Oct 4, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Oct 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sipsorcery sipsorcery Awaiting requested review from sipsorcery

1 more reviewer

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hebasto @DrahtBot @sipsorcery @Zhaojun-Liu @maflcko