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

Move model instances into one source file, so they get initialized in the correct order #467

Merged
merged 1 commit into from
Apr 12, 2017

Conversation

ChrisDodd
Copy link
Contributor

No description provided.

@sethfowler
Copy link
Contributor

In the long term, the right fix here is to minimize our use of global variables and not depend implicitly on static initialization order.


/* These must be in the same compiliation unit to ensure that P4CoreLibrary::instance
* is initialized before V1Model::instance */
namespace P4V1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

The name of this cpp file should be changed.
It should perhaps be called "modelInstances.cpp"

Copy link
Member

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

I confirm that this fixes the issue which was originally reported by the LLVM Address Sanitizer. However, as @sethfowler pointed out, I think these global variables should be avoided by using a singleton pattern or by using global raw pointers.

@mihaibudiu
Copy link
Contributor

This was meant to be a singleton pattern, but I guess in C++ you have to do much more work to create one.

@sethfowler
Copy link
Contributor

It's not really that much work. =) Both of the approaches Antonin suggested require only a couple of lines of code - for the raw pointer approach, all you need to do is explicitly initialize the variable somewhere before it's used, while for the singleton pattern you just need to wrap accesses to the variable in a function and perform initialization in that function. Call sites need to be updated in both cases, though.

@ChrisDodd
Copy link
Contributor Author

renamed the file to modelInstances.cpp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants