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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
