clementval 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"},
----------------
awarzynski wrote:
> 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.
>
> > This is counter intuitive.
>
> I can add a comment to clarify this.
>
There should be at least a comment 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.
>
If it cannot be configurable I think this is a good reason to wait a bit and
make a direct renaming without all the options and duplicate.
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
