awarzynski added a comment. @richard.barton.arm @CarolineConcatto thank you both for clarification! @CarolineConcatto , if you prefer to go ahead with this approach - could you please address all the outstanding comments? I've left a few comments that I'd like addressed first, but nothing major.
Also, IIUC, the approach that we are taking here is via a _custom_ fortran frontend. I'm looking at `ccc_gcc_name` as reference, which I believe stands for `custom c compiler`, see: - link1 <https://github.com/llvm/llvm-project/blob/42a56bf63f699a620a57c34474510d9937ebf715/clang/lib/Driver/ToolChains/Gnu.cpp#L183> - link2 <http://lists.llvm.org/pipermail/cfe-dev/2011-June/015622.html> - link3 <http://clang-developers.42468.n3.nabble.com/ccc-gcc-name-equivalent-to-call-as-and-ld-directly-td3076391.html> I couldn't find any other documentation other than ^^^. Anyway, I think that it would be useful to make it clear that this flag points to either a _custom_ frontend (e.g. `cfc_flang_name`), or perhaps _alternative_ frontend (e.g. `afc_flang_name`). Currently the name is a bit too generic IMO (i.e. we want to make the intention for the flag very clear). Last minor point - I think that this patch is more about _specfying_ rather than _searching_ `flang` frontend. It would be helpful to have this clarified in the commit message. Thanks for working on this! :) ================ Comment at: clang/test/Driver/flang/clang-driver-2-frontend01.f90:7 + +! Copy clang to two temporary file +! t1 is the new frontend name. ---------------- 1. You make only one copy here :) 2. In this file `%t` should be sufficient (i.e. you don't need `%t1`). 3. IIUC, you don't need to copy here at all. Instead of using `%basename_t.tmp1` below you could use `alternative_fortran_frontend` (or some other name). I would prefer if we avoided copying in these tests. It's a step that complicates the test, yet doesn't add anything in terms of test coverage. ================ Comment at: clang/test/Driver/flang/clang-driver-2-frontend01.f90:10 +! RUN: cp %clang %t1 +! RUN: %clang --driver-mode=flang -fortran-fe %basename_t.tmp1 -### %s 2>&1 | FileCheck --check-prefixes=ALL %s + ---------------- richard.barton.arm wrote: > CarolineConcatto wrote: > > richard.barton.arm wrote: > > > Does %t1 not work again on this line? > > If I don't create the fake link getProgramPath will only return the name, > > not the entire path. > > t1 here is the path for the frontend. > > For instance: > > clang --driver-mode=flang -fortran-fe %test > > the frontend name is test, but when running it should print: > > <path-to-test>/test -fc1 > > without the link it will only print: > > test -fc1 > > Like I said before it is more a preference that actually a requisite. > Understood - thanks. There's only one run line here, so `--check-prefixes` is not required here. Could you please use the default label instead, i.e. `CHECK`? ================ Comment at: clang/test/Driver/flang/flang-driver-2-frontend01.f90:7-8 +! Copy clang to a temporary file to be the driver name +! RUN: cp %clang %t1 +! RUN: %t1 --driver-mode=flang -### %s 2>&1 | FileCheck --check-prefixes=ALL %s + ---------------- IIUC, copying is not required here. This could be simplified as: ``` ! RUN: %clang --driver-mode=flang -### %s 2>&1 | FileCheck --check-prefixes=ALL %s ``` Also, this scenario is already tested in `flang.90`. I'd rather we didn't add this file. ================ Comment at: clang/test/Driver/flang/flang-driver-2-frontend02.f90:2 +! Check wich name of flang frontend is invoked by the driver + +! The flag -fortran-fe is passed by the driver. ---------------- This test is very similar to `clang-driver-2-frontend01.f90` - the scenario tested here is already covered elsewhere and IMO this file can be removed from this patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73951/new/ https://reviews.llvm.org/D73951 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
