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

[Manuel Klimek]

On Tue, Mar 6, 2012 at 11:22 PM, Marshall Clow <mclow.lists at gmail.com> wrote:
> 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.

The main thing that bothered me about that code is that it was
actually only needed in one place, so I inlined it :)

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

Interesting. When I am reading code and see an object passed as a
pointer, I see that as a sign that the pointer is retained past the
call of the function. When using references in that case it's
unfortunately not visible from the call site that the reference is
retained.

Please find an updated patch attached...

Cheers,
/Manuel

>
> 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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tooling.patch
Type: application/octet-stream
Size: 47225 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120307/a6e4a365/attachment.obj>