[libcxx-commits] [PATCH] D133082: [clang] Implement setting crash_diagnostics_dir through env variable

[Matheus Izvekov via Phabricator via libcxx-commits]

mizvekov added inline comments.


================
Comment at: clang/lib/Driver/Driver.cpp:5425
   Arg *A = C.getArgs().getLastArg(options::OPT_fcrash_diagnostics_dir);
-  if (CCGenDiagnostics && A) {
-    SmallString<128> CrashDirectory(A->getValue());
+  const char *CrashDirectory = CCGenDiagnostics && A
+                                   ? A->getValue()
----------------
erichkeane wrote:
> mizvekov wrote:
> > erichkeane wrote:
> > > `StringRef` would be better here instead, which should mean you don't have to create the SmallString below, and could just work in `Twine`s for everything.
> > It seems that `llvm::sys::path::append` takes twine as inputs, but it only outputs to a SmallVectorImpl.
> > Unless there is something else I could use?
> Ah, yikes, I  missed that was what was happening :/  THAT is still necessary.  A Twine can be created from a stringref (and I'd still prefer one anyway), but you DO still have to make that SmallString unfortunately.
The Twine also can't take a nullptr (such as the case no flag passed and no env variable), so we would have to use something like `std::optional<Twine>`, and make the needed conversions here.

And the Twine to SmallString copy looks unergonomic as well.

So it seems we would need to make some little changes out of scope of this MR to get that to look nicely.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133082/new/

https://reviews.llvm.org/D133082