[PATCH] D125788: [flang][driver] Rename `flang-new` as `flang`

[Valentin Clement via Phabricator via cfe-commits]

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