On Mon, Aug 12, 2013 at 8:46 AM, Thompson, John < [email protected]> wrote:
> Manuel,**** > > ** ** > > Thanks for the feedback.**** > > ** ** > > >first, I'd be eternally thankful if you submitted patches to Tooling via > phabricator (http://llvm.org/docs/Phabricator.html) :)**** > > ** ** > > I’ll give it a try.**** > > ** ** > > >Second, I'm not yet convinced this patch is the right direction. I'm > trying to understand what you're doing:**** > > >1. when running a tool over code, I'd assume you don't need to change the > include file arguments**** > > ** ** > > Modularize reads a list of headers to individually compile and check. > Some of these headers might depend on other headers to be included first. > This allows modularize to force the compiler to read first the headers that > are depended on, so the main input header file will compile without errors. > **** > > ** ** > > >2. after run modularize, if I understand you correctly, the header search > paths are now not correct any more - this looks to me like it needs build > system support, rather than trying to work around this with the include > paths**** > > ** ** > > This doesn’t involve changing include paths. The -include file options > added tell the compiler to read those files first before compiling the main > input file. There is no build system involved other than running > modularize via a command line.**** > > ** ** > > The alternative is either to create a separate ClangTool object with the > added -include options for each header file to be checked, or change > ClangTool to let me provide individual compile commands, probably needing a > need CompilationDatabase class. Using ArgumentsAdjuster seems to be a more > elegant approach. For example, here’s the new modularize code showing the > interaction with ClangTool:**** > > ** ** > > // We provide this derivation to add in "-include (file)" arguments for > header**** > > // dependencies.**** > > class AddDependenciesAdjuster : public ArgumentsAdjuster {**** > > public:**** > > AddDependenciesAdjuster(DependencyMap &Dependencies)**** > > : Dependencies(Dependencies) {}**** > > private:**** > > CommandLineArguments Adjust(llvm::StringRef InputFile,**** > > const CommandLineArguments &Args) {**** > > DependentsVector &FileDependents = Dependencies[InputFile];**** > > int Count = FileDependents.size();**** > > if (Count == 0)**** > > return Args;**** > > CommandLineArguments NewArgs(Args);**** > > for (int Index = 0; Index < Count; ++Index) {**** > > NewArgs.push_back("-include");**** > > std::string File(**** > > std::string("\"") + FileDependents[Index] + std::string("\""));*** > * > > NewArgs.push_back(FileDependents[Index]);**** > > }**** > > return NewArgs;**** > > }**** > > DependencyMap &Dependencies;**** > > };**** > > ** ** > > …**** > > ** ** > > // Parse all of the headers, detecting duplicates.**** > > EntityMap Entities;**** > > ClangTool Tool(*Compilations, Headers);**** > > Tool.appendArgumentsAdjuster(new AddDependenciesAdjuster(Dependencies)); > **** > > int HadErrors =**** > > Tool.run(new ModularizeFrontendActionFactory(Entities, *PPTracker)); > **** > > ** ** > > Or would using a separate ClangTool instance for each header be actually > more in line with how the tooling system should work? > Ok, after giving it some thought, I think your use cases makes sense - but can't you figure out the input file from the given args? > **** > > ** ** > > -John**** > > ** ** > > ** ** > > ** ** > > ** ** > > *From:* Manuel Klimek [mailto:[email protected]] > *Sent:* Monday, August 12, 2013 6:34 AM > *To:* Thompson, John > *Cc:* [email protected] > *Subject:* Re: {PATCH] clang::tooling::ArgumentAdjuster - Add file name > argument to Adjust callback**** > > ** ** > > Hi John,**** > > ** ** > > first, I'd be eternally thankful if you submitted patches to Tooling via > phabricator (http://llvm.org/docs/Phabricator.html) :)**** > > ** ** > > Second, I'm not yet convinced this patch is the right direction. I'm > trying to understand what you're doing:**** > > 1. when running a tool over code, I'd assume you don't need to change the > include file arguments**** > > 2. after run modularize, if I understand you correctly, the header search > paths are now not correct any more - this looks to me like it needs build > system support, rather than trying to work around this with the include > paths**** > > ** ** > > Cheers,**** > > /Manuel**** > > ** ** > > ** ** > > On Mon, Aug 12, 2013 at 6:16 AM, Thompson, John < > [email protected]> wrote:**** > > This patch adds an input file name parameter to ArgumentAdjuster’s Adjust > function in Tooling, in order to allow support of changes to a compile’s > arguments based on the file name.**** > > **** > > I’m in a testing phase for a new feature for modularize which allows you > to add dependencies to the header files in the input header list. In using > modularize for a real-world set of headers, I found that some headers > needed other headers to be included first, as opposed to including them > themselves or relying on a master header to include the subset of headers > in the right order. With this patch, I can create an ArgumentAdjuster > derivation that’s given a map with the dependencies keyed on the file name, > and have the Adjust function look up the dependencies using the input file > name and then add -include options to first include the depended-on > headers. This seemed easier than other changes or derivations that would > need to be done.**** > > **** > > Thanks.**** > > **** > > -John**** > > **** > > **** > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits**** > > ** ** >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
