erichkeane accepted this revision.
erichkeane 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()
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D133082/new/
https://reviews.llvm.org/D133082
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
