[cfe-commits] [PATCH] Implements support to run standalone tools

[Manuel Klimek]

On Mar 29, 2012 5:25 PM, "Marshall Clow" <mclow.lists at gmail.com> wrote:
>
> On Mar 29, 2012, at 4:49 AM, Manuel Klimek wrote:
>
> > Updated patch: now injecting the CompilationDatabase into the
> > ClangTool, plus some clean ups.
> >
> > Please let me know
> > - if that's in line with what you thought
>
> Yes, this is more in line with what I was thinking.
>
> But… ;-)
>
> I think that:
>
> 1)
> +  /// FIXME: Currently only supports JSON compilation databases, which
> +  /// are named 'compile_commands.json' in the given directory. Extend this
> +  /// for other build types (like ninja build files).
> +  static CompilationDatabase *loadFromDirectory(StringRef BuildDirectory,
> +                                                std::string &ErrorMessage);
>
> doesn't belong in CompilationDatabase.
> The reason is that in my suggested usage, there will not be a "build directory" to load the compilation database from.
> I agree that JSONCompilationDatabase and "MakefileCompilationDatabase" (or whatever) need this, but for simple tools, I'd like to be able to specify all of the options on the command line.
> Things like:
>        find . -name \*.cpp -exec tool -option1 -option2 -option3 {} \;

I'm not clear whether you're saying the design I proposed a few mails
ago is not a good goal (which is pretty much what I now implemented,
re-pasting to make sure we're talking about the same reference):
<define tool command lines>
int main(...) {
  // Make argc and argv references so we can change them? Kind of
yuck, alternative ideas welcome.
  llvm::OwningPtr<CompilationDatabase>
Database(createSimpleCompilationDatabase(argc, argv));
  <do llvm command line parsing on rest of argc, argv>
  if (!Database) {
    Database.reset(loadCompilationDatabase(BuildDirectory));
  }
  ClangTool Tool(Database.get(), Filenames);
}

... or whether you're saying that I should implement the
"createSimpleCompilationDatabase(argc, argv)" right away? (which I'm
fine with, but I don't want to bloat this patch with more features)

The reason I think the creation of the SimpleCompilationDatabase (I'm
not happy with the name yet) should be different from the
CompilationDatabase::loadFromDirectory(...) is that I want to keep the
normal llvm command line parsing outside of the CompilationDatabase,
as otherwise it gets hard to add extra parameters when writing a tool,
which is pretty common.

Thoughts?
/Manuel

> 2) runSyntaxOnlyToolOnCode
> I don't understand why "input.cc" is hard wired here.
> Is this just some dummy file name - supposed to be unique?
> Maybe a comment to say what's going on?
>
> -- Marshall
>
>
> > - what else I need to do before landing this :)
> >
> > Cheers,
> > /Manuel
> >
> > On Thu, Mar 29, 2012 at 10:53 AM, Manuel Klimek <klimek at google.com> wrote:
> >> Updated patch where I pulled out CompilationDatabase.h/.cpp/_test.cpp,
> >> to make sure we're all on the same page.
> >>
> >> Next step is to dependency inject it into the ClangTool and fix the
> >> command line args handling.
> >>
> >> Cheers,
> >> /Manuel
> >>
> >> On Mon, Mar 26, 2012 at 4:43 PM, Douglas Gregor <dgregor at apple.com> wrote:
> >>>
> >>> On Mar 26, 2012, at 7:39 AM, Manuel Klimek <klimek at google.com> wrote:
> >>>
> >>> On Mon, Mar 26, 2012 at 4:26 PM, Douglas Gregor <dgregor at apple.com> wrote:
> >>>>
> >>>>
> >>>> On Mar 26, 2012, at 7:21 AM, Manuel Klimek <klimek at google.com> wrote:
> >>>>
> >>>> On Mon, Mar 26, 2012 at 3:56 PM, Douglas Gregor <dgregor at apple.com> wrote:
> >>>>>
> >>>>>
> >>>>> On Mar 26, 2012, at 4:08 AM, Manuel Klimek <klimek at google.com> wrote:
> >>>>>
> >>>>>
> >>>>>>> Then again, when you say "CompilationDatabase", I think of something
> >>>>>>> that I can extract a set { commands, arguments, source file } tuples to be
> >>>>>>> executed.
> >>>>>>> Maybe that's where we are are talking past each other.
> >>>>>>
> >>>>>> When I say CompilationDatabase I think of a map<filename, { command,
> >>>>>> arguments, path}>. Does that make sense?
> >>>>>
> >>>>>
> >>>>> Makes sense to me, although of course this could end up being a multimap.
> >>>>
> >>>>
> >>>> Which is also an interesting design question - at least for refactorings
> >>>> we'll probably want to run over all different command lines the file was
> >>>> compiled with (think: different configurations with different macros
> >>>> enabled). This would lead to a slightly different interface for
> >>>> CompilationDatabase though (returning a list of compile command lines for
> >>>> each target).
> >>>>
> >>>>
> >>>> I think it's important for us to support this case in the interface.
> >>>
> >>>
> >>> Agreed. So something like:
> >>>
> >>> /// \brief Specifies the working directory and command of a compilation.
> >>> struct CompileCommand {
> >>>   /// \brief The working directory the command was executed from.
> >>>   std::string Directory;
> >>>
> >>>   /// \brief The command line that was executed.
> >>>   std::vector<std::string> CommandLine;
> >>> };
> >>>
> >>> class CompilationDatabase {
> >>> public:
> >>>   /// \brief Returns all compile commands in which the specified file was
> >>> compiled.
> >>>   ///
> >>>   /// This includes compile commands that span multiple source files.
> >>>   /// For example, for a compile command line
> >>>   /// $ clang++ -o test a.cc b.cc t.cc
> >>>   /// $ clang++ -o production a.cc b.cc -DPRODUCTION
> >>>   /// A compilation database representing the project would return both
> >>> command lines
> >>>   /// for a.cc and b.cc and only the first command line for t.cc.
> >>>   virtual vector<CompileCommand> getCompileCommands(StringRef FilePath) = 0;
> >>> };
> >>>
> >>>
> >>> Sure. Returning vectors is cheap now! ;)
> >>>
> >>> Also, I'd like to pull CompilationDatabase into its own header file if that
> >>> is fine with you.
> >>>
> >>>
> >>> Yes, please.
> >>>
> >>> - Doug
> >>>
> > <tooling.patch>
>
> -- Marshall
>
> Marshall Clow     Idio Software   <mailto:mclow.lists at gmail.com>
>
> A.D. 1517: Martin Luther nails his 95 Theses to the church door and is promptly moderated down to (-1, Flamebait).
>        -- Yu Suzuki
>