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