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

[Marshall Clow]

On Feb 13, 2012, at 1:05 AM, Manuel Klimek wrote:

> Ping.
> 
> Rebased patch + fixed some nits from Nick.


Two concerns in Tooling.cpp:

1)
> 	/// \brief Converts a string vector representing a Command line into a C
> 	/// string vector representing the Argv (including the trailing NULL).
> 	std::vector<char*> commandLineToArgv(ArrayRef<std::string> Command) {
> 	  std::vector<char*> Result(Command.size() + 1);
> 	  for (std::vector<char*>::size_type I = 0, E = Command.size(); I != E; ++I) {
> 	    Result[I] = const_cast<char*>(Command[I].c_str());
> 	  }
> 	  Result[Command.size()] = NULL;
> 	  return Result;
> 	}
> 
This code does two things that bother me.
a) It calls c_str() on a bunch of strings, and then saves the pointers. The pointers that result from calling c_str() are only valid as long as the underlying strings are unchanged. 
b) Then the code casts away the const-ness of the pointers. If someone attempts to change those strings, that's undefined behavior.


2)
> 	bool ToolInvocation::runInvocation(
> 	    const char *BinaryName,
> 	    clang::driver::Compilation *Compilation,
> 	    clang::CompilerInvocation *Invocation,
> 	    const clang::driver::ArgStringList &CC1Args,
> 	    clang::FrontendAction *ToolAction) {
> 
> 	  llvm::OwningPtr<clang::FrontendAction> ScopedToolAction(ToolAction);
> 	  // Show the invocation, with -v.
> 	  if (Invocation->getHeaderSearchOpts().Verbose) {
> 	    llvm::errs() << "clang Invocation:\n";
> 	    Compilation->PrintJob(llvm::errs(), Compilation->getJobs(), "\n", true);
> 	    llvm::errs() << "\n";
> 	  }
> 


When I am reading (and writing) code, and I see an object passed as a pointer, I figure that it's optional; it can be NULL - otherwise it would be passed by reference.

Other than that, this patch LGTM!

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