Minor clean-up in the plugin infrastructure. #4895
Conversation
include/aspect/plugins.h
Outdated
| @@ -301,8 +301,7 @@ namespace aspect | |||
| // if any plugins have been registered, then delete | |||
| // the list | |||
| if (plugins != nullptr) | |||
| delete plugins; | |||
| plugins = nullptr; | |||
| plugins.reset(); | |||
There was a problem hiding this comment.
can you remove the if here and unconditionally call .reset()?
|
So done. |
|
There is also a compile error. 😉 |
include/aspect/plugins.h
Outdated
| @@ -181,7 +181,7 @@ namespace aspect | |||
| * need not worry whether we try to add something to this list before | |||
| * the lists's constructor has successfully run | |||
| */ | |||
| static std::list<PluginInfo> *plugins; | |||
| static std::unique_ptr<std::list<PluginInfo>> plugins; | |||
There was a problem hiding this comment.
Does the comment above still apply regarding initialization order? unique_ptr does have a constructor.
|
Oh, I had totally forgotten about the reason why we used pointers. Thanks for digging this out of the comments! This invalidates the whole idea and probably explains why the original idea resulted in all sorts of segfaults on the testers (though interestingly not on my laptop). I've stripped the patch back to the minimal pieces that keep things as a pointer. |
|
Didn't have to do much digging, it was right there above the declaration. I actually looked because I was wondering why we have a pointer instead of a std::list directly. Another way to work around the awkward pointer would be to have a getter that returns a reference to a static std::list. |
include/aspect/plugins.h
Outdated
| description, | ||
| declare_parameters_function, | ||
| factory_function)); | ||
| plugins->emplace_back (PluginInfo(name, |
There was a problem hiding this comment.
| plugins->emplace_back (PluginInfo(name, | |
| plugins->emplace_back (name, |
Otherwise emplace_back will do the same as push_back.
There was a problem hiding this comment.
Yes, good eye! So fixed!
|
I was lazy and only suggested the opening paren and not both. :-) |
|
Ah, good grief. That's what happens when you work on your laptop and are working on the one file that is literally included into every single .h file and for which compiling is going to fry your knees. Not even |
|
I think ASPECT is s pleasure to develop because of the fast compilation times. Always annoyed when I have to recompile deal.II... |
|
Neither on my laptop. But there really also isn't all that much you can do when you're mucking around with the most central pieces. You're entirely right when working on plugins or individual (Why not unity: Because not using unity is fastest when you're working on a single |
|
Try both some day. Recompiling one of the unity files when a single .cc changes is also surprisingly fast. |
A companion to #4891.
/rebuild