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