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

[Matheus Izvekov via Phabricator via cfe-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:
> > mizvekov wrote:
> > > erichkeane wrote:
> > > > mizvekov wrote:
> > > > > 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.
> > > > I guess i don't get the concern here?  I'm suggesting just changing this from `const char*` to `StringRef` (which should work).  Everything else below should 'just work' AFAIK.
> > > Oh okay I misunderstood you! Thought you meant to make CrashDirectory a Twine.
> > > 
> > > Yeah with `StringRef` we only need to change the test:
> > > 
> > > ```
> > > if (CrashDirectory) {
> > > ```
> > > --->
> > > ```
> > > if (CrashDirectory.data() != nullptr) {
> > > ```
> > > 
> > > Everything else stays the same. It doesn't look cleaner, but would address your concerns about avoiding storing a char pointer variable?
> > We could add a bool conversion to StringRef, but perhaps folks could find the meaning confusing between empty and null pointer?
> Hrm... do we actually have meaning here to 'empty' for these files?  I guess we allow it already.... hrm... Ok, disregard the suggestion, and thanks for thinking this through with me.
Thanks for the review and suggestions!

Yeah right, I didn't want to change the behavior vs empty here, but I think if we did some change, it would make more sense to create some specific error for it, instead of treating empty the same as no argument passed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133082