Skip to content
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.

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

[kernel 3d/n] Misc ChainstateManager::Options fixups #25607

Merged
merged 2 commits into from
Jul 14, 2022

Conversation

dongcarl
Copy link
Contributor

@dongcarl dongcarl commented Jul 13, 2022

This is part of the libbitcoinkernel project: #24303, https://github.com/bitcoin/bitcoin/projects/18

This PR is NOT dependent on any other PRs.


Places ChainstateManager::Options into the kernel:: namespace and use designated initializers for construction.

@dongcarl
Copy link
Contributor Author

@MarcoFalke wondering if you know why the Desig usages are not working here? https://cirrus-ci.com/task/6347551300386816?logs=ci#L2321

init.cpp does #include <util/designator.h>

@maflcko
Copy link
Member

maflcko commented Jul 14, 2022

You'll need to rebase 😅

It should have been there in the first place.
This wasn't available at the time when ChainstateManager::Options was
introduced but is helpful to be explicit and ensure correctness.
@dongcarl
Copy link
Contributor Author

Intermittent rpc_net issue strikes again! https://cirrus-ci.com/task/6461219992240128?logs=ci#L4516

(restarting CI job)

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Code review ACK ce8b0f9

I didn't realize Desig(field_name) compiled to nothing on certain platforms. That seems like it was dangerous because code might appear to be initializing one field, but actually be initializing another if fields were not in order. Unless maybe field order was enforced through CI. Anyway glad that is gone now.

@maflcko
Copy link
Member

maflcko commented Jul 14, 2022

Yes, it was enforced on every platform except msvc.

@maflcko maflcko merged commit 02ede4f into bitcoin:master Jul 14, 2022
@ryanofsky
Copy link
Contributor

Yes, it was enforced on every platform except msvc.

Thanks, I didn't realize out of order designated initializers were actually forbidden by the language.

@ryanofsky
Copy link
Contributor

Thanks, I didn't realize out of order designated initializers were actually forbidden by the language.

Though if skipping a field was allowed this still could have been bad.

@maflcko
Copy link
Member

maflcko commented Jul 14, 2022

Skipping is allowed, and seems no worse than omission of "trailing" fields

@ryanofsky
Copy link
Contributor

Wouldn't this assign (1, 2, 0) on msvc and (1, 0, 2) on other platforms?

struct S {
   int a;
   int b;
   int c;
};

S s{
  Desig(a) 1,
  Desig(c) 2,
};

@maflcko
Copy link
Member

maflcko commented Jul 14, 2022

Yes (oops)

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 14, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Jul 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Status: Done or Closed or Rethinking
Development

Successfully merging this pull request may close these issues.

None yet

3 participants