{PATCH] clang::tooling::ArgumentAdjuster - Add file name argument to Adjust callback

Tue Aug 13 00:41:34 PDT 2013

On Mon, Aug 12, 2013 at 11:20 AM, Thompson, John <
John_Thompson at playstation.sony.com> wrote:

>  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?
>

I think putting the input file in as a special case isn't what we want - if
we do that, what do we do if we need to extract other options when we
modify them?

If we really need more tools to extract different parts of the options, I
think the right way is to make parsing those options easier via a small
library function.

Cheers,
/Manuel

> ****
>
> ** **
>
> Thanks.****
>
> ** **
>
> -John****
>
> ** **
>
> *From:* Manuel Klimek [mailto:klimek at google.com]
> *Sent:* Monday, August 12, 2013 9:16 AM
>
> *To:* Thompson, John
> *Cc:* cfe-commits at cs.uiuc.edu
> *Subject:* Re: {PATCH] clang::tooling::ArgumentAdjuster - Add file name
> argument to Adjust callback****
>
> ** **
>
> On Mon, Aug 12, 2013 at 8:46 AM, Thompson, John <
> John_Thompson at playstation.sony.com> 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:klimek at google.com]
> *Sent:* Monday, August 12, 2013 6:34 AM
> *To:* Thompson, John
> *Cc:* cfe-commits at cs.uiuc.edu
> *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 <
> John_Thompson at playstation.sony.com> 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
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits****
>
>  ****
>
>  ** **
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130813/cee2f54c/attachment.html>