dblaikie 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;
+
----------------
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?


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

Reply via email to