awarzynski added inline comments.
================ Comment at: clang/lib/Driver/ToolChain.cpp:185 {"flang", "--driver-mode=flang"}, + {"flang-new", "--driver-mode=flang"}, {"clang-dxc", "--driver-mode=dxc"}, ---------------- richard.barton.arm wrote: > clementval wrote: > > This is counter intuitive. We rename flang-new but we add flang-new here. > > It should be configurable. > This is a list of all the valid prefixes of binaries that the driver > supports. With this change, an additional one will be supported so I don't > think it's an issue to have both flang and flang-new here. > > The thing that confuses me is how flang-new works today without this change, > given that "flang" is not a suffix of "flang-new"? @awarzynski , do you know > why that is? Perhaps the line here is not needed at all? Or is this a bug fix > for flang-new that is actually unrelated to this change? > This is counter intuitive. I can add a comment to clarify this. > It should be configurable. It is, in Flang's [[ https://github.com/llvm/llvm-project/blob/main/flang/tools/flang-driver/driver.cpp | driver.cpp ]]. Originally, the suffix was hard-coded as: ``` clang::driver::ParsedClangName targetandMode("flang", "--driver-mode=flang"); ``` (i.e. the `clangDriver` library used `flang` internally despite the actual name being `flang-new`). This is now being replaced with (see in "driver.cpp"): ``` clang::driver::ParsedClangName targetandMode = clang::driver::ToolChain::getTargetAndModeFromProgramName(argv[0]); ``` But the change in "driver.cpp" means that we can no longer make assumptions about the suffix and hence the update here. Like I mentioned earlier, we should not make this file build-time configurable. One possible option would be to try to update Clang's [[ https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Config/config.h.cmake | config.h.cmake ]], but that's would lead to more Flang-specific changes in Clang's core set-up. Also, I'm not convinced it would work here. > @awarzynski , do you know why that is? Yeah, check Flang's "driver.cpp". We should've captured this earlier. The original set-up from ToolChain.cpp predates `flang-new`. And then in "driver.cpp" we just matched what was here. There was a lot of confusion around naming back then and this has slipped in. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125788/new/ https://reviews.llvm.org/D125788 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits