[PATCH] D73951: [Clang] [Driver]Add logic to search for flang frontend

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm added inline comments.


================
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">;
----------------
CarolineConcatto wrote:
> richard.barton.arm wrote:
> > 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.
> I really don't mind changing the name. The way it is stands ffc for: flang fc1
> As I thought that ccc was for clang cc1
> As I thought that ccc was for clang cc1

Hmm - that might be right right. It is certainly as good a guess as any.
I was also thinking maybe it was "Cross CC" or something like that given the use of the word "native" in the help text.

Perhaps we should just not use the naming convention as we don't really understand it. Maybe `-flang-fc1` or `-fortran-fe` might be a better name?

Anyhow - given what you say, I'll withdraw my quibbling on the name. It is an internal name after all. Happy to go with whatever you chose, including the original.


================
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
----------------
CarolineConcatto wrote:
> richard.barton.arm wrote:
> > 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?
> Mainly for sanity check as the patch add a new flag. The logic added does not mess with the previous flags, but I thought it was good to do at least a minimum check.
> 
> If changing I would add more tests like the ones made in flang.f90 and flang.F90 as the one in these files do not use ffc-fortran-name.
I agree with you that the check is correct, -emit-obj will indeed be emitted so the check is a correct one for clang's current behaviour.

Where I am coming from is that there are already tests that check that when clang is called without `-c` (or `-S`, etc.) that it passes `-emit-obj` to the frontend. This test should be checking that `-ffc-fortran-name` causes the frontend that clang calls to change. It's not really concerned with the `-emit-obj` behaviour. But, because you have this CHECK line in the code, the test depends on it to pass. Say we changed the spelling of that option -emit-obj or we changed the behaviour of clang in some way that causes `-emit-obj` no longer to be passed to the FE in these circumstances. This test would still fail if not updated, even though it is supposed to be unrelated to the feature being changed. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73951/new/

https://reviews.llvm.org/D73951