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:
> 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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D133082/new/
https://reviews.llvm.org/D133082
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
