[PATCH] D86089: [flang][driver]Add experimental flang driver and frontend with help screen

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm requested changes to this revision.
richard.barton.arm added a comment.

A few specific comments to address here as well as the pre-commit linting ones.



================
Comment at: clang/lib/Driver/Driver.cpp:1584
 void Driver::PrintVersion(const Compilation &C, raw_ostream &OS) const {
+  if (IsFlangMode()) {
+    OS << "Flang experimental driver (flang-new)" << '\n';
----------------
Instead of this early exit, could we instead of calling getClangFullVersion below call getClangToolFullVersion with a different string here?


```
if (IsFlangMode())
  OS >> getClangToolFullVersion("flang-new") << '\n';
else
  OS >> getClangFullVersion();
```

That way, we get the full clang version screen already implemented 'for free' and the code is nicer too (IMO)
  


================
Comment at: flang/CMakeLists.txt:20
 option(LINK_WITH_FIR "Link driver with FIR and LLVM" ON)
+option(BUILD_FLANG_NEW_DRIVER "Build the flang driver frontend" OFF)
 
----------------
Generally we should make sure all Flang-specific CMake build configuration variables start with FLANG. Suggest this is FLANG_BUILD_NEW_DRIVER or similar.


================
Comment at: flang/CMakeLists.txt:74
 
+
   if(LINK_WITH_FIR)
----------------
Remove


================
Comment at: flang/test/lit.cfg.py:40
+# exclude the tests for flang_new driver while there are two drivers
+if config.include_flang_new_driver_test == "OFF":
+  config.excludes = ['Inputs', 'CMakeLists.txt', 'README.txt', 'LICENSE.txt', 'Flang-Driver']
----------------
I think it would be cleaner to define config.excludes unconditionally then append the Flang-Driver dir if our condition passes.


================
Comment at: flang/test/lit.cfg.py:64
 tools = [
+  ToolSubst('%flang-new', command=FindTool('flang-new'), unresolved='ignore'),
   ToolSubst('%f18', command=FindTool('f18'),
----------------
Rather than always trying to add flang-new and ignoring failure, I think it would be better to conditionally add this to `tools` iff we are building the new driver and so have `config.include_flang_new_driver_test = "ON"`. This way if you are building the new driver and for some reason lit fails to resolve the tool then you get a nice error before trying to run tests on a binary that is not there.


================
Comment at: flang/test/lit.site.cfg.py.in:13
 
+# controld the regression test for flang-new driver
+config.include_flang_new_driver_test="@BUILD_FLANG_NEW_DRIVER@"
----------------
typo "controld"


================
Comment at: flang/unittests/CMakeLists.txt:27
+if (BUILD_FLANG_NEW_DRIVER)
+add_subdirectory(Frontend)
+endif()
----------------
indentation?


================
Comment at: llvm/lib/Option/OptTable.cpp:621
+    // If `Flags` is empty (i.e. it's an option without any flags) then this is
+    // a Clang-only option. If:
+    // * we _are not_ in Flang Mode (FlagsToExclude contains FlangMode), then
----------------
This is not the sort of change I expect to see in an llvm support library given that it seems very clang and flang specific. 

I think this needs to be re-written to be more generic, perhaps tweaking the interface to be able to express the logic you want to use for flang and clang.

Why is it not sufficient to call this function unchanged from both flang and clang but with a different set of FlagsToInclude/FlagsToExclude passed in using this logic to set that on the clang/flang side?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86089