h-vetinari added inline comments.
================
Comment at: clang/lib/Driver/Driver.cpp:5639-5640
+
+ // If we're emitting a module output with the speicifed option
+ // `-fmodule-output`.
+ if (!AtTopLevel && isa<PrecompileJobAction>(JA) &&
----------------
================
Comment at: clang/lib/Driver/Driver.cpp:5736
+ C.getArgs().hasArg(options::OPT_fmodule_output) &&
+ C.getArgs().hasArg(options::OPT_o)) {
+ SmallString<128> OutputPath;
----------------
dblaikie wrote:
> ChuanqiXu wrote:
> > dblaikie wrote:
> > > tahonermann wrote:
> > > > dblaikie wrote:
> > > > > ChuanqiXu wrote:
> > > > > > dblaikie wrote:
> > > > > > > Is there some way we can avoid this (`-fmodule-output -o ...`)
> > > > > > > being a special case? It'd be good if we could use a single
> > > > > > > common implementation regardless of whether `-o` is used (ie:
> > > > > > > Figure out the output file name (whether it's implicitly
> > > > > > > determined by clang, in the absence of `-o`, or explicitly
> > > > > > > provided by the user through `-o`) and then replace the suffix
> > > > > > > with `pcm`)?
> > > > > > I guess we can't do it or we can't do it easily. Otherwise the .pcm
> > > > > > file will always lives in the same directory with the input file.
> > > > > > I guess we can't do it or we can't do it easily. Otherwise the .pcm
> > > > > > file will always lives in the same directory with the input file.
> > > > >
> > > > > I don't follow/understand. What I mean is, I'd like it, if possible,
> > > > > this was implemented by something like "find the path for the .o file
> > > > > output, then replace the extension with .pcm".
> > > > >
> > > > > I worry atht code like this that recomputes something similar to the
> > > > > object output path risks getting out of sync with the actual object
> > > > > path.
> > > > That is certainly a valid concern and I agree it would be better if the
> > > > shared output path is computed exactly once. If that would require
> > > > significant change, then tests to ensure consistent behavior would be
> > > > the next best thing. I'm not sure what infrastructure is in place for
> > > > validation of output file locations though.
> > > Well, I guess the Split DWARF file naming isn't fundamentally better - it
> > > looks at the OPT_O argument directly too:
> > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/CommonArgs.cpp#L1250
> > >
> > > Perhaps we could at least reuse this same logic - make it a reusable
> > > function in some way? (for instance, it checks `-c` too, which seems
> > > relevant - we wouldn't want to name the .pcm after the linked executable
> > > name, right?
> > >
> > > Perhaps we could at least reuse this same logic - make it a reusable
> > > function in some way?
> >
> > Done. It looks better now.
> >
> > > (for instance, it checks -c too, which seems relevant - we wouldn't want
> > > to name the .pcm after the linked executable name, right?
> >
> > Oh, right. Although the previous conclusion is that if `-o` is specified,
> > the .pcm file should be in the same dir with the output. But it is indeed
> > weird that the .pcm file are related to the linked executable if we thought
> > it deeper.
> Ah, I was hoping it could reuse the same code, rather than duplicate it - any
> chance it could be refactored into a common implementation between Split
> DWARF and modules?
> Ah, I was hoping it could reuse the same code, rather than duplicate it - any
> chance it could be refactored into a common implementation between Split
> DWARF and modules?
Could we uncouple this clean-up from landing the patches before the LLVM 16
branch? A trivial refactor might even still be backportable, but the whole
series will be more challenging.
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
