richard.barton.arm requested changes to this revision.
richard.barton.arm added a comment.
A few comments running through that need addressing.
In addition - have you checked the behaviour of this option with `-Bprefix`?
Looking at the code for Driver.GetProgramPath, it seems like that would affect
the behaviour of this feature so `clang --driver-mode=flang -B<prefix>
-ffc-fortran-name=foo` might be expected to make clang call `<prefix>foo`.
There also seems to be some logic with `-target` that might affect this. There
are tests for this feature in test/Driver/B-opt.c that could be added to or
adapted into a new test.
================
Comment at: clang/include/clang/Driver/Options.td:264
MetaVarName<"<gcc-path>">;
+def fcc_fortran_name : Separate<["-"], "ffc-fortran-name">, InternalDriverOpt,
+ HelpText<"Name for native Fortran compiler">;
----------------
I'm not sure about this name. My memory is not long enough to know what ccc
stands/stood for but git blame suggests it may have been a precursor name to
clang. Although it might seem odd to add new options like this given the name
perhaps doesn't mean anything anymore, I suggest copying this convention and
make this option start with `-ccc`. I also think flang would be better than
fortran here as it better describes what the option is doing, i.e. telling
clang what flang is called.
So recommend `-ccc-flang-name` for the option and commensurate changes to the
internal variables to match.
================
Comment at: clang/test/Driver/flang/clang-driver-2-frontend01.f90:12
+! t1 is the new frontend name.
+! RUN: cp %clang %t1
+! RUN: %clang --driver-mode=flang -ffc-fortran-name %basename_t.tmp1 -### %s
2>&1 | FileCheck --check-prefixes=ALL,CHECK-EMIT-OBJ %s
----------------
Given that the RUN line runs clang with `-###` I don't think it is necessary
for the flang binary that `-ffc-fortran-name` points to actually be present.
Can't we simplify the test for the basic case to run with
`-ffc-fortran-name=foo` and check for foo?
If it is required that the `-ffc-fortran-name` argument point to an executable
file that exists it would be better to create files in the Inputs directory and
point there.
================
Comment at: clang/test/Driver/flang/clang-driver-2-frontend01.f90:14
+! RUN: %clang --driver-mode=flang -ffc-fortran-name %basename_t.tmp1 -### %s
2>&1 | FileCheck --check-prefixes=ALL,CHECK-EMIT-OBJ %s
+! CHECK-EMIT-OBJ-DAG: "-emit-obj"
+! CHECK-EMIT-OBJ-DAG: "-o" "{{[^"]*}}.o
----------------
I don't understand why you are checking for this option. Surely the only
relevant check is the "ALL-LABEL" check at the top which checks for the flang
subprocess invocation? Can you explain why you need this check?
================
Comment at: clang/test/Driver/flang/clang-driver-2-frontend01.f90:18
+! Should end in the input file.
+! ALL: "{{.*}}clang-driver-2-frontend01.f90"{{$}}
----------------
Similarly, I don't understand the need for this check on the output, although
it would be correct.
================
Comment at: clang/test/Driver/flang/driver-2-frontend01.f90:3
+
+! Driver name is a randon name. It does not contain flag, flang or clang,
+! therefore the driver invokes flang FE.
----------------
Why does the driver name matter here. Unless I have overlooked something I
would expect the functionality to be the same whatever the driver is called.
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
