-
Notifications
You must be signed in to change notification settings - Fork 2
Eliminate singleton configuration with custom FrontendActionFactory. #281
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
This PR eliminates the need for the singleton instantiation of the chimera::Configuration by implementing custom factory classes that allow the configuration to be passed to the ASTConsumer directly.
| if(BUILD_TESTING) | ||
| enable_testing() | ||
| add_subdirectory(test EXCLUDE_FROM_ALL) | ||
| endif() |
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 saw that there was a BUILD_TESTING flag being used, but it didn't seem to affect the build. I needed to build without tests (I didn't have python 3.7 installed), so I added this option.
Feel free to suggest a different name for the flag.
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.
BUILD_TESTING is automatically set when we include CTest. I think the issue is that CTest is being included in test/examples/CMakeLists.txt (line: 1). Let's replace line 174 with include(CTest) and remove line 1 of test/examples/CMakeLists.txt.
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.
Does this look like what you had in mind?
| */ | ||
| std::unique_ptr<clang::tooling::FrontendActionFactory> newFrontendActionFactory( | ||
| const chimera::Configuration &config); | ||
|
|
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.
This approach is summarized in this comment thread requesting this functionality be added to LLVM. It doesn't look like it was added though :(
http://lists.llvm.org/pipermail/cfe-dev/2014-December/040607.html
jslee02
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.
Left a few suggestions
| if(BUILD_TESTING) | ||
| enable_testing() | ||
| add_subdirectory(test EXCLUDE_FROM_ALL) | ||
| endif() |
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.
BUILD_TESTING is automatically set when we include CTest. I think the issue is that CTest is being included in test/examples/CMakeLists.txt (line: 1). Let's replace line 174 with include(CTest) and remove line 1 of test/examples/CMakeLists.txt.
…mera into no_singleton_config
* Add pybind11 examples -- incomplete * Generate binding for Pet::getName() * Add `dynamic_attr` to enable dynamic attributes * Update comment * Remove unnecessary changes
|
Ok, I think I made all the changes now. |
jslee02
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.
Looks good to me!
This PR eliminates the need for the singleton instantiation of the
chimera::Configurationby implementing custom factory classes that allow the configuration to be passed to the ASTConsumer directly.