ChuanqiXu marked an inline comment as done.
ChuanqiXu added inline comments.
================
Comment at: clang/lib/Driver/Driver.cpp:5736
+ C.getArgs().hasArg(options::OPT_fmodule_output) &&
+ C.getArgs().hasArg(options::OPT_o)) {
+ SmallString<128> OutputPath;
----------------
h-vetinari wrote:
> 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.
> 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?
I feel it is an overkill to merge these 2 logics. It will become harder to read
and modify the logics after we merge them. I feel better with the current
implementation since it has fewer code dependencies and its complexity is low.
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
