[PATCH] D95460: [flang][driver] Add forced form flags and -ffixed-line-length

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thank you for working on this @FarisRehman ! I've left a few inline comments, but nothing major. Really nice to see this being added and working as expected!



================
Comment at: clang/include/clang/Driver/Options.td:4074-4076
+def ffixed_line_length_VALUE : Joined<["-"], "ffixed-line-length-">, Flags<[FlangOption, FC1Option]>, 
+  HelpText<"Set column after which characters are ignored in typical fixed-form lines in the source file">,
+  Group<gfortran_Group>;
----------------
This is a fairly long help message. This is what `gfortran` prints:
```
gfortran --help=joined | grep 'ffixed-line-length'
  -ffixed-line-length-<n>     Use n as character line width in fixed mode
```
A long version could be added using `DocBrief` field.


================
Comment at: clang/include/clang/Driver/Options.td:4110-4111
 defm f2c : BooleanFFlag<"f2c">, Group<gfortran_Group>;
-defm fixed_form : BooleanFFlag<"fixed-form">, Group<gfortran_Group>;
-defm free_form : BooleanFFlag<"free-form">, Group<gfortran_Group>;
+defm fixed_form : BooleanFFlag<"fixed-form">, Flags<[FlangOption, FC1Option]>, Group<gfortran_Group>;
+defm free_form : BooleanFFlag<"free-form">, Flags<[FlangOption, FC1Option]>, Group<gfortran_Group>;
 defm frontend_optimize : BooleanFFlag<"frontend-optimize">, Group<gfortran_Group>;
----------------
We can't have help text for these, can we? Perhaps worth mentioning in the commit message that this needs fixing. We are likely to experience this with other options too.


================
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:24-26
+  Args.AddAllArgs(CmdArgs, {options::OPT_D, options::OPT_U, options::OPT_I,
+                            options::OPT_ffixed_form, options::OPT_ffree_form,
+                            options::OPT_ffixed_line_length_VALUE});
----------------
IIUC, you are adding Fortran dialect rather then preprocessor options here. See also `gfortran` docs: https://gcc.gnu.org/onlinedocs/gfortran/Fortran-Dialect-Options.html.


================
Comment at: flang/include/flang/Frontend/FrontendOptions.h:77-87
+enum class FortranForm {
+  /// The user has not specified a form. Base the form off the file extension.
+  Unknown,
+
+  /// -ffree-form
+  FixedForm,
+
----------------
[nit] Perhaps `Source file layout` above `enum class FortranForm`?


================
Comment at: flang/lib/Frontend/CompilerInstance.cpp:151-158
+      if (invoc.frontendOpts().fortranForm_ == FortranForm::Unknown) {
+        // Switch between fixed and free form format based on the input file
+        // extension. Ideally we should have all Fortran options set before
+        // entering this loop (i.e. processing any input files). However, we
+        // can't decide between fixed and free form based on the file extension
+        // earlier than this.
+        invoc.fortranOpts().isFixedForm = fif.IsFixedForm();
----------------
Hm, unwanted TABs? 

Also, please keep note of: https://reviews.llvm.org/D95464 (there should be no conflicts from what I can tell).


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:166-171
+    if (currentArg->getOption().matches(
+            clang::driver::options::OPT_ffixed_form)) {
+      opts.fortranForm_ = FortranForm::FixedForm;
+    } else {
+      opts.fortranForm_ = FortranForm::FreeForm;
+    }
----------------
Could be simplified using the ternary operator:
```
opts.fortranForm_ = (currentArg->getOption().matches(clang::driver::options::OPT_ffixed_form) ? FortranForm::FixedForm : FortranForm::FreeForm;
```
To me this would be more concise, but semantically it's identical. So please decide!

(similar comment for the bit below)


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:182
+      columns = 0;
+    } else if (argValue.getAsInteger(10, columns)) {
+      columns = -1;
----------------
Please, could you comment hard-coded args:
```
 } else if (argValue.getAsInteger(/*Radix=*/10, columns)) {
```
See [[ https://llvm.org/docs/CodingStandards.html#comment-formatting | LLVM's coding style ]]


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:319-323
+  if (frontendOptions.fortranForm_ == FortranForm::FixedForm) {
+    fortranOptions.isFixedForm = true;
+  } else if (frontendOptions.fortranForm_ == FortranForm::FreeForm) {
+    fortranOptions.isFixedForm = false;
+  }
----------------
Hm, wouldn't this work:
```
fortranOptions.isFixedForm = (frontendOptions.fortranForm_ == FortranForm::FixedForm) ? true : false;
 ```


================
Comment at: flang/test/Flang-Driver/Inputs/fixed-line-length-test.f:4
+      end
\ No newline at end of file

----------------
FIXME


================
Comment at: flang/test/Flang-Driver/fixed-free-flag.f90:8-9
+!--------------------------
+! RUN: not %flang-new -fsyntax-only -ffree-form %S/Inputs/fixed-form-test.f %S/Inputs/free-form-test.f90  2>&1 | FileCheck %s --check-prefix=FREEFORM
+! RUN: %flang-new -fsyntax-only -ffixed-form %S/Inputs/free-form-test.f90 %S/Inputs/fixed-form-test.f  2>&1 | FileCheck %s --check-prefix=FIXEDFORM
+
----------------
Current `RUN` lines are quite complicated and there's a lot going on. It might be safer to simplify them. Perhaps this:
```
! RUN: not %flang-new -fsyntax-only -ffree-form %S/Inputs/fixed-form-test.f  2>&1 | FileCheck %s --check-prefix=ERROR
! RUN: not %flang-new -fsyntax-only -ffixed-form %S/Inputs/free-form-test.f90  2>&1 | FileCheck %s --check-prefix=ERROR
```
So:
* `-ffree-form` + fixed form file (extension and contents) --> Failure
* `-fixed-form` + free form file (extension and contents) --> Failure

This is way you indeed test that the driver is forced to assume particular layout? 


================
Comment at: flang/test/Flang-Driver/fixed-line-length.f90:27
+! The line should be trimmed to 72 characters when reading based on the default value of fixed line length.
+! DEFAULTLENGTH: programaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+
----------------
Could this be a regex instead?


================
Comment at: flang/test/Flang-Driver/fixed-line-length.f90:32
+!-----------------------------------------
+! INVALIDLENGTH: invalid value '{{(-)?[0-9]+}}'
+
----------------
The value is know, right? I would added it here. Also, the actual diagnostics printed is longer, isn't it?

Would it be possible to test negative numbers?


================
Comment at: flang/test/Flang-Driver/fixed-line-length.f90:38
+! The line should not be trimmed and so 73 characters (including spaces) should be read.
+! UNLIMITEDLENGTH: programaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+
----------------
Regex?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95460