[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

Tue Jul 26 01:57:25 PDT 2022

awarzynski added a comment.

Thanks for working on this. This is not my area of expertise, so I focused on the implementation in the driver.

================
Comment at: clang/include/clang/Driver/Options.td:4897
 def ffixed_line_length_VALUE : Joined<["-"], "ffixed-line-length-">, Group<f_Group>, Alias<ffixed_line_length_EQ>;
+def fconvert_EQ : Joined<["-"], "fconvert=">, Group<f_Group>,
+  HelpText<"Set endian conversion of data for unformatted files">;
----------------
jpenix-quic wrote:
> peixin wrote:
> > Why do you move it here? Maybe it is not implemented now, clang may need this option eventually. @MaskRay 
> I was using the fixed line length options as a reference for how to handle this--based on the discussion in the review here (https://reviews.llvm.org/D95460) about forwarding options to gfortran, I was thinking that it would also be safe to handle fconvert similarly, but I'm not 100% sure and definitely might be misunderstanding something!
GFortran support has been effectively broken since LLVM 12 (see e.g. this [[ https://github.com/llvm/llvm-project/commit/6a75496836ea14bcfd2f4b59d35a1cad4ac58cee | change ]]). We would do ourselves a favor if we just removed it altogether (I'm not aware of anyone requiring  it). 

And if Clang ever needs this option, we can always update this definition accordingly. No need to optimize for hypothetical scenarios that may or may not happen :) To me, this change makes perfect sense.

================
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:52
+                            options::OPT_fno_automatic,
+                            options::OPT_fconvert_EQ});
 }
----------------
jpenix-quic wrote:
> Reading through https://reviews.llvm.org/D95460 again, I'm not sure this is the appropriate place to add . I am marking this as a TODO that I will revisit with the other feedback!
You can use `AddOtherOptions` instead. `AddFortranDialectOptions` is more about language options. Is this a language option? It's a bit of a borderline. Feel free to add another hook too.

Btw, the reformatting is an unrelated change. Could you submit in a separate patch? Thanks!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130513