[PATCH] D25257: Use StringRef in Option library instead of raw pointers (NFC)

[Zachary Turner via llvm-commits]

zturner added a comment.

I guess my comment applies everywhere there is a call to `.data()`.  Suppose you shelve this CL, then go change some of the places where you are using `.data()` in this CL to accept `StringRef` arguments, then rebase this CL on top of those.  Since those functions have been left alone in this CL, I would guess that many of them are independent of the changes in this CL and could be converted to `StringRef` first (and in much smaller CLs), then when you come back to this one you won't have to use `.data()` so much.

As mentioned in the first comment though, whenever `.data()` is necessary, I would tend to favor `.str().c_str()`  (the fact that it is inefficient compared to `.data()` could serve as more incentive to go fix the other places first so that you don't have to do either one)



> Compilation.cpp:105
>           it = Files.begin(), ie = Files.end(); it != ie; ++it)
> -    Success &= CleanupFile(*it, IssueErrors);
> +    Success &= CleanupFile(it->data(), IssueErrors);
>    return Success;

In general, I prefer `it->str().c_str()` over `data()` almost everywhere.  Yes it's slower, but we shouldn't be making any assumptions about the underlying `StringRef`.  And anyway, in this particular case we're dealing with the filesystem.  An extra alloc isn't goign to hurt.

> Driver.cpp:618
>        OS << ' ';
> -    Command::printArg(OS, *I, true);
> +    Command::printArg(OS, I->data(), true);
>    }

Can you change `Command::printArg` to take a `StringRef`?  If it's too hard to change it outright, you could add an overload.

> Driver.cpp:796
>    std::string TmpName = GetTemporaryPath("response", "txt");
> -  Cmd.setResponseFile(
> -      C.addTempFile(C.getArgs().MakeArgString(TmpName.c_str())));
> +  Cmd.setResponseFile(C.addTempFile(C.getArgs().MakeArgString(TmpName)).data());
>  }

Can `setResponseFile` take a `StringRef`?

> Driver.cpp:1168
>      for (unsigned i = 0, e = Archs.size(); i != e; ++i)
> -      Inputs.push_back(C.MakeAction<BindArchAction>(Act, Archs[i]));
> +      Inputs.push_back(C.MakeAction<BindArchAction>(Act, Archs[i].data()));
>  

Can `BindArchAction` take a `StringRef`?

> Driver.cpp:1310
> +          if (ExtIdx != StringRef::npos)
> +            Ty = TC.LookupTypeForExtension(Value.data() + ExtIdx + 1);
>  

Can `LookupTypeForExtension` take a `StringRef`?

> Driver.cpp:1350
> +            const char *Ext = Value.data() + ExtIdx + 1;
> +            if (TC.LookupTypeForExtension(Ext) == types::TY_Object)
> +              Ty = types::TY_Object;

Same as above.

> Driver.cpp:2314
>                         /*MultipleArchs*/ ArchNames.size() > 1,
> -                       /*LinkingOutput*/ LinkingOutput, CachedResults,
> +                       /*LinkingOutput*/ LinkingOutput.data(), CachedResults,
>                         /*BuildForOffloadDevice*/ false);

Same here.

https://reviews.llvm.org/D25257