PCM generation now uses the ROOT modulemap to build modules. #504
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Starting build on |
Teemperor
force-pushed
the
NewModuleMapGeneration
branch
from
013a0fa to
55e4e37
Compare
pcanal
reviewed
Apr 13, 2017
core/dictgen/src/rootcling_impl.cxx
Outdated
| /// needed by the given ModuleGenerator. | ||
| /// The names of all header files that are needed by the ModuleGenerator but are | ||
| /// not in the given module will be inserted into the MissingHeader variable. | ||
| static bool moduleContainsHeaders(TModuleGenerator &modGen, clang::Module *module, |
Member
There was a problem hiding this comment.
Coding convention calls for CamelCase for funcitons names. thanks.
Teemperor
force-pushed
the
NewModuleMapGeneration
branch
from
55e4e37 to
95d1a38
Compare
pcanal
reviewed
Apr 13, 2017
core/dictgen/src/rootcling_impl.cxx
Outdated
| /// respecting the given isysroot. | ||
| /// If module is not a null pointer, we only write the given module to the | ||
| /// given file and not the whole AST. | ||
| int writeAST(StringRef FileName, clang::CompilerInstance *CI, StringRef ISysRoot, clang::Module *module = nullptr) |
Member
There was a problem hiding this comment.
please use CamelCase for function names. thanks.
Teemperor
force-pushed
the
NewModuleMapGeneration
branch
from
95d1a38 to
9ffaa42
Compare
vgvassilev
reviewed
Apr 13, 2017
Member
vgvassilev
left a comment
vgvassilev
left a comment
There was a problem hiding this comment.
Overall looks good modulo minor comments.
| // Write the generated bitstream to "Out". | ||
| OS->write((char *)&Buffer.front(), Buffer.size()); | ||
| //////////////////////////////////////////////////////////////////////////////// | ||
| /// Returns true iff a given module (and its submodules) contains all headers |
Member
There was a problem hiding this comment.
core/dictgen/src/rootcling_impl.cxx
Outdated
| int GenerateModule(TModuleGenerator &modGen, clang::CompilerInstance *CI) | ||
| { | ||
| // modGen must not be in PCH mode. Otherwise this is a programming error. | ||
| assert(!modGen.IsPCH()); |
Member
There was a problem hiding this comment.
Could you add && "Comment" to the assert?
core/dictgen/src/rootcling_impl.cxx
Outdated
|
|
||
| // Inform the user and abort if we can't find a module with a given name. | ||
| if (module == nullptr) { | ||
| std::cerr << "error: Couldn't find module with name '" << ModuleName << "' in modulemap!\n"; |
Member
There was a problem hiding this comment.
We should ROOT's error reporting facilities, eg Error.
Member
There was a problem hiding this comment.
ROOT::TMetaUtils::Error to be more precise.
core/dictgen/src/rootcling_impl.cxx
Outdated
|
|
||
| // Free up some memory, in case the process is kept alive. | ||
| Buffer.clear(); | ||
| // Check if the loaded modules covers all headers that were specified |
core/dictgen/src/rootcling_impl.cxx
Outdated
| std::vector<std::string> MissingHeaders; | ||
| if (!moduleContainsHeaders(modGen, module, MissingHeaders)) { | ||
| // FIXME: Upgrade this to an error once modules are stable. | ||
| std::cerr << "warning: Couldn't find the following specified headers in " |
core/dictgen/src/rootcling_impl.cxx
Outdated
| std::cerr << "warning: Couldn't find the following specified headers in " | ||
| << "the module " << module->Name << ":\n"; | ||
| for (auto &H : MissingHeaders) { | ||
| std::cerr << " " << H << "\n"; |
core/dictgen/src/rootcling_impl.cxx
Outdated
|
|
||
| //////////////////////////////////////////////////////////////////////////////// | ||
| /// Generates a module from the given ModuleGenerator and CompilerInstance. | ||
| int GenerateModule(TModuleGenerator &modGen, clang::CompilerInstance *CI) |
Member
There was a problem hiding this comment.
What about returning 'bool', it seems we check only for success.
Member
|
@martinmine, it looks like the build failed due to configuration issues. Could you have a look, this is on a separate branch called |
Teemperor
force-pushed
the
NewModuleMapGeneration
branch
3 times, most recently
from
48afdf8 to
9897568
Compare
This patch also untangles the generation code of PCMs and PCHs. Additionally we enable rootcling warnings during module generation because want them in the future when we try to adjust the working ROOT modulemap to the dictionary layout (i.e. fixing libCore and libThread).
Member
|
@phsft-bot build! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch also untangles the generation code of PCMs and PCHs.