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

[Faris via Phabricator via llvm-commits]

FarisRehman added inline comments.


================
Comment at: flang/lib/Frontend/FrontendAction.cpp:60
+        pathSuffix == "ff" || pathSuffix == "for" || pathSuffix == "FOR" ||
+        pathSuffix == "fpp" || pathSuffix == "FPP";
+  }
----------------
tskeith wrote:
> awarzynski wrote:
> > 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`.
> > 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.
> 
> If `GetInputKindForExtension` contained code like:
> `if (IsFixedFormSuffix(suffix) || IsFreeFormSuffix(suffix)) { return Language::Fortran; }`
> it would suggest to me that source form is not significant as they are both being treated the same way.
> 
> > Also, we would have to abandon the `llvm::StringSwitch` API there, which IMO fits there very well.
> 
> It would fit better if the API weren't so clumsy. Wanting to use a favorite class is not a reason to duplicate code.
> 
> I'm fine with a different approach, but we shouldn't be listing the fixed-form suffixes twice in different places.
> 
>From this discussion, it seems in this case moving away from StringSwitch and having helper methods isFixedFormSuffix and isFreeFormSuffix is the best solution. I have made these changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94228