[PATCH] D94228: [flang][driver] Support fixed form detection

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added inline comments.


================
Comment at: flang/lib/Frontend/FrontendAction.cpp:60
+        pathSuffix == "ff" || pathSuffix == "for" || pathSuffix == "FOR" ||
+        pathSuffix == "fpp" || pathSuffix == "FPP";
+  }
----------------
tskeith wrote:
> This set of fixed form suffixes is duplicated in `FrontendOptions.cpp`. I think it would be better two have two predicates, say `IsFixedFormSuffix` and `IsFreeFormSuffix`, so that the former can be used in both places.
I would prefer if we kept it as is.

In FrontendOptions.cpp (in particular in `GetInputKindForExtension`), we don't intend to and should not differentiate between the fixed and free forms. Nor should we leave any comments that would suggest otherwise. `GetInputKindForExtension` is about differentiating between e.g. `Fortran`, `CUDA`, `LLVM IR` inputs. For reference, see similar method in Clang:
* https://github.com/llvm/llvm-project/blob/cbea6737d5130724c7c8cf8ee4ccf1c3dd099450/clang/lib/Frontend/FrontendOptions.cpp#L15-L36.

If something like `IsFixedFormSuffix` and `IsFreeFormSuffix` is introduced and used in `GetInputKindForExtension`, then that will suggest that the differentiation between fixed and free form is significant in that method. That's not the case. Also, we would have to abandon the `llvm::StringSwitch` API there, which IMO fits there very well.

The only reason that there are two cases for `Language::Fortran` in `GetInputKindForExtension` is that LLVM's `Cases` allows 9 entries at most: 
* https://github.com/llvm/llvm-project/blob/cbea6737d5130724c7c8cf8ee4ccf1c3dd099450/llvm/include/llvm/ADT/StringSwitch.h#L132-L137
There's more than 9 file extensions that will lead to `Language::Fortran`.


================
Comment at: flang/lib/Frontend/FrontendOptions.cpp:21
           Language::Fortran)
+      .Cases("ff", "FOR", "for", "f77", "ff90", "fpp", "FPP", Language::Fortran)
       .Default(Language::Unknown);
----------------
FarisRehman wrote:
> sameeranjoshi wrote:
> > Why is this repeated from above `Cases` statement?
> > Why are all options not inside a single `Cases` statement?
> > If it's an issue mentioned from the comments, would it be better to differentiate the above options in 2 different `Cases` based on free and fixed form and add a one liner comment on top about the form they belong to?
> Correct, it is because of the issue mentioned in the comments.
> I agree and I have made the changes to separate the cases based on form, thanks.
@sameeranjoshi @FarisRehman 

Could I suggest that we actually remove ` // Free form files` and `// Fixed form files`? These comments suggest that the split into fixed and free form matters here, but that is not the case here (see also my reply to @tskeith 's comment).

The key comment here is this:
```
      // FIXME: Currently this API allows at most 9 items per case.
```
Link to the API: https://github.com/llvm/llvm-project/blob/cbea6737d5130724c7c8cf8ee4ccf1c3dd099450/llvm/include/llvm/ADT/StringSwitch.h#L132-L137. This is the only reason why we have two cases here instead of one. Perhaps we should just update that comment:
```
//  llvm::StringSwithc::Cases allows at most 9 strings per entry and hence we need multiple cases for Language::Fortran.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94228