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

Thu Jan 14 04:24:28 PST 2021

FarisRehman marked 2 inline comments as done.
FarisRehman added inline comments.

================
Comment at: flang/lib/Frontend/FrontendOptions.cpp:21
           Language::Fortran)
+      .Cases("ff", "FOR", "for", "f77", "ff90", "fpp", "FPP", Language::Fortran)
       .Default(Language::Unknown);
----------------
awarzynski wrote:
> 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.
> ```
Based on how the discussion evolved, I also feel that it is best to not have these comments here, and I have removed them. cc @sameeranjoshi 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94228