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

[Tim Keith via Phabricator via llvm-commits]

tskeith added inline comments.


================
Comment at: flang/lib/Frontend/FrontendAction.cpp:60
+        pathSuffix == "ff" || pathSuffix == "for" || pathSuffix == "FOR" ||
+        pathSuffix == "fpp" || pathSuffix == "FPP";
+  }
----------------
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94228