[PATCH] D137058: [Driver] [Modules] Support -fmodule-output (1/2)

[David Blaikie via Phabricator via cfe-commits]

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