ChuanqiXu added inline comments.
================
Comment at: clang/lib/Driver/Driver.cpp:5541-5545
+ if (Arg *FinalOutput = C.getArgs().getLastArg(options::OPT_o))
+ TempPath = FinalOutput->getValue();
+ else
+ TempPath = BaseInput;
+
----------------
dblaikie wrote:
> dblaikie wrote:
> > ChuanqiXu wrote:
> > > dblaikie wrote:
> > > > It'd be nice if we didn't have to recompute this/lookup `OPT_o` here -
> > > > any way we can use the object file output path here (that's already
> > > > handled `OPT_o` or using the base input name, etc)?
> > > I didn't understand this a lot. We don't compute anything here and we
> > > just use the object file output path here if `-o` is provided and we
> > > replace the suffix then. I feel this is simple enough.
> > Computing the path to write to is what I'm referring to - and the fact that
> > this had a bug (was relative to the source path instead of the
> > CWD)/divergence from the `-o` path logic is the sort of thing I want to
> > avoid.
> >
> > I'm not sure there's an easy way to do this - but looks like the logic to
> > name the .o file output is in `Driver::GetNamedOutputPath` and gets stored
> > in the `clang::driver::Compilation`.... which I guess is where we are, but
> > we're going through here with a `PrecompileJobAction` insntead of the
> > compile job action, I suppose.
> >
> > Could we keep these two codepaths closer together?
> >
> > It'd be really nice if we could reuse that in some way.
> >
> > Hmm, actually, why doesn't this fall out of the existing algorithm without
> > modification?
> >
> > Ah, I see, since the precompile action isn't "at top level" it gets a
> > temporary file name - so if we change only that, it seems to fall out
> > naturally:
> > ```
> > diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp
> > index c7efe60b2335..db878cbfff46 100644
> > --- a/clang/lib/Driver/Driver.cpp
> > +++ b/clang/lib/Driver/Driver.cpp
> > @@ -5556,9 +5556,9 @@ const char *Driver::GetNamedOutputPath(Compilation
> > &C, const JobAction &JA,
> > }
> >
> > // Output to a temporary file?
> > - if ((!AtTopLevel && !isSaveTempsEnabled() &&
> > + if (((!AtTopLevel && !isSaveTempsEnabled() &&
> > !C.getArgs().hasArg(options::OPT__SLASH_Fo)) ||
> > - CCGenDiagnostics) {
> > + CCGenDiagnostics) && JA.getType() != types::TY_ModuleFile) {
> > StringRef Name = llvm::sys::path::filename(BaseInput);
> > std::pair<StringRef, StringRef> Split = Name.split('.');
> > const char *Suffix = types::getTypeTempSuffix(JA.getType(),
> > IsCLMode());
> > ```
> >
> > Without the need to reimplement/duplicate the `-o` handling logic?
> Oh, I should say, this patch didn't actually have the flag support, but it'd
> be something like this ^ but with the command line argument test as well (so
> "other stuff that's already there && !(TY_ModuleFile && hasArg
> fmodule-output)")
To be honest, I prefer the previous patch. I feel it has higher readability.
But this is a problem about taste and it doesn't have standard answer.
Someone's readability is redundancy for others : )
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137058/new/
https://reviews.llvm.org/D137058
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
