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

[Caroline via Phabricator via cfe-commits]

CarolineConcatto marked 17 inline comments as done.
CarolineConcatto added a comment.

Hey Folks,
Fist of all, thank you very much for your review. 
I have answered some questions and update some of the requests. 
There are some that I did not had time to go inside yet.
I have the week of 24th off. IN my return I will tackle all the reviews as:
 out of build tree 
 and some question about the driver



================
Comment at: flang/include/flang/Frontend/CompilerInstance.h:11
+
+#include "flang/Frontend/CompilerInvocation.h"
+
----------------
tskeith wrote:
> Why is this called "Frontend" rather than "Driver"?
The way we think is that the driver is split in:
**the driver** libDriver -> that implements the driver. ATM it is inside clang/lib/Driver. And it can be clang( called inside clang/tools/driver/driver.cpp) or flang-new(called inside flang/tools/driver/driver.cpp) according to the mode the driver is called.
**the frontend driver**  -> it can be:
     clang frontend driver: clang -cc1  that calls libclangfrontend 
     flang frontend driver: flang-new -fc1 that calls libflangfrontend. 
the -xc1 has its functions/methods implemented inside the driver Frontend 

So this folder is called Frontend because it belongs to the part that implements the driver frontend of the compiler and we would like to leave this way so it has the same design as clang.
 


================
Comment at: flang/include/flang/Frontend/CompilerInstance.h:21
+  /// The options used in this compiler instance.
+  std::shared_ptr<CompilerInvocation> Invocation;
+
----------------
tskeith wrote:
> Data members, local variables, etc. should start with lower case.
> Non-public data members should end with underscore.
> 
> Please follow the style in flang/documentation/C++style.md.
I believe I have changed the style of the patch to be flang style.


================
Comment at: flang/include/flang/Frontend/FrontendOptions.h:31
+  Language Lang;
+  unsigned Fmt : 3;
+  unsigned Preprocessed : 1;
----------------
tskeith wrote:
> Why isn't the type `Format`?
This is to not mistake with: enum Format { Source, ModuleMap, Precompiled };


================
Comment at: flang/include/flang/Frontend/FrontendOptions.h:32
+  unsigned Fmt : 3;
+  unsigned Preprocessed : 1;
+
----------------
tskeith wrote:
> Why isn't the type `bool`?
Most of these things are going to be set by option::driver.  That is the reason it is as it is.


================
Comment at: flang/include/flang/Frontend/FrontendOptions.h:65
+  /// Show the -version text.
+  unsigned ShowVersion : 1;
+
----------------
tskeith wrote:
> Why aren't the types `bool`?
Most of these things are going to be set by option::driver. That is the reason it is as it is.


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:46
+      InputKind DashX(Language::Unknown);
+      return DashX;
+    }
----------------
tskeith wrote:
> Why not `return InputKind{Language::Unknown};`?
Just because the next lines also return DashX.


================
Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:20
+
+using namespace flang;
+namespace flang {
----------------
tskeith wrote:
> What is this for?
Because of CompilerInstance *Flang.


================
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']
----------------
richard.barton.arm wrote:
> richard.barton.arm wrote:
> > I think it would be cleaner to define config.excludes unconditionally then append the Flang-Driver dir if our condition passes.
> I _think_ the pattern to follow to exclude tests for something you haven't built is to use lit features.
> 
> So you would add a feature like:
> `config.available_features.add("new-driver")`
> 
> then each test that only works on the new driver would be prefixed with a statement:
> 
> `REQUIRES: new-driver`
> 
> This means that the tests can go in the test/Driver directory and you don't need to create a new directory for these tests or this hack. The additional benefit would be that all the existing tests for the throwaway driver can be re-used on the new Driver to test it. There are not many of those though and we are using a different driver name so they can't be shared either. Still think it would be a worthwhile thing to do because when looking at the test itself it is clear why it is not being run whereas with this hack it is hidden away.
> 
>  Sorry for not thinking this first time around.
I like to have the test at a different folder for now so it is clear that the tests inside this folder all belongs to the new driver. So I don't need to open the test to know.
I can implement the requires, but in the future will not need the REQUIRES for the driver test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86089