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:[email protected]>
A.D. 1517: Martin Luther nails his 95 Theses to the church door and is promptly
moderated down to (-1, Flamebait).
-- Yu Suzuki
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
