Son of a gun, I thought I looked and didn't see the input file in the Args, but in checking again, there it is.
However, this patch does make it easier, allowing me to avoid writing logic to determine which argument is the input file, since ClangTool had the input file handy already. But I'll defer to you. Do you think it's helpful enough to put in, or would avoiding the change be better? Thanks. -John From: Manuel Klimek [mailto:[email protected]] Sent: Monday, August 12, 2013 9:16 AM To: Thompson, John Cc: [email protected] Subject: Re: {PATCH] clang::tooling::ArgumentAdjuster - Add file name argument to Adjust callback On Mon, Aug 12, 2013 at 8:46 AM, Thompson, John <[email protected]<mailto:[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]<mailto:[email protected]>] Sent: Monday, August 12, 2013 6:34 AM To: Thompson, John Cc: [email protected]<mailto:[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]<mailto:[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]<mailto:[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
