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

[Thompson, John]

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?

-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<mailto: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<mailto: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/20130812/eb82cb1e/attachment.html>