-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Simplify plugin registrations #13139
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
Conversation
xokdvium
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Diff LGTM
|
@edolstra Formatter is not happy :( |
|
Looks like this made initialization of classes somehow more lazy so they are no longer initialized in the manual generation. Overall it seems useful if this makes the loading order more deterministic but we need to fix the manual then. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how this is a simplification, because it still relies on the mutation of globals, and the amount of code seems to be about the same.
Feel free to go ahead though, assuming it's an incremental improvement. (if the other comments can be addressed)
|
Yes the end state has to be no globals, but from a quick glance this does feel to me like an improvement. |
It's a simplification because it replaces static Commands * commands;
...
RegisterCommand::Commands * RegisterCommand::commands = nullptr;
...
if (!commands)
commands = new Commands;(i.e. 3 different sites and a conditional), with just static Commands & commands()
{
static Commands commands;
return commands;
} |
Afaik it provides more thread-safety during initialization and we have to deal with less raw pointer. |
|
Just want to add that the merge above was triggered by auto-merge, that I have set up initially. I saw your comments after the fact. |
|
I'm still investigating, but I think this broke nix on cygwin, because it depends on |
|
Yeah, it was an issue with MacOS as well. We need to move the singletons into the libraries. It's sad that non ELF platforms have issues with "vague" linkage. |
|
How did you work around this on MacOS? I was going to attempt moving the data as you suggested. Edit: do you mean 060c34b ? The problem I ran into was with |
|
Yeah, I did suggest moving all the other static variables from headers to .cc files, but seems like we forgot some. |
|
I fixed what I could find in #14551. |
Motivation
Use Meyer's singleton pattern in our various registration classes, as suggested by @xokdvium (#13138 (comment)).
Context
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.