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

[Tim Keith via Phabricator via llvm-commits]

tskeith requested changes to this revision.
tskeith added inline comments.
This revision now requires changes to proceed.


================
Comment at: flang/include/flang/Frontend/CompilerInstance.h:11
+
+#include "flang/Frontend/CompilerInvocation.h"
+
----------------
Why is this called "Frontend" rather than "Driver"?


================
Comment at: flang/include/flang/Frontend/CompilerInstance.h:16
+
+namespace flang {
+
----------------
Nothing else is in namespace `flang`. `Fortran::frontend` would be more consistent.


================
Comment at: flang/include/flang/Frontend/CompilerInstance.h:21
+  /// The options used in this compiler instance.
+  std::shared_ptr<CompilerInvocation> Invocation;
+
----------------
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.


================
Comment at: flang/include/flang/Frontend/FrontendOptions.h:31
+  Language Lang;
+  unsigned Fmt : 3;
+  unsigned Preprocessed : 1;
----------------
Why isn't the type `Format`?


================
Comment at: flang/include/flang/Frontend/FrontendOptions.h:32
+  unsigned Fmt : 3;
+  unsigned Preprocessed : 1;
+
----------------
Why isn't the type `bool`?


================
Comment at: flang/include/flang/Frontend/FrontendOptions.h:65
+  /// Show the -version text.
+  unsigned ShowVersion : 1;
+
----------------
Why aren't the types `bool`?


================
Comment at: flang/include/flang/FrontendTool/Utils.h:18
+
+#include <memory>
+
----------------
Is this needed?


================
Comment at: flang/lib/Frontend/CompilerInstance.cpp:39
+  } else
+    Diags->setClient(new clang::TextDiagnosticPrinter(llvm::errs(), Opts));
+
----------------
The "then" and "else" statements should have braces around them.


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:46
+      InputKind DashX(Language::Unknown);
+      return DashX;
+    }
----------------
Why not `return InputKind{Language::Unknown};`?


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:91
+  InputKind DashX = ParseFrontendArgs(Res.getFrontendOpts(), Args, Diags);
+  (void)DashX;
+
----------------
What is the purpose of `DashX` here?


================
Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:20
+
+using namespace flang;
+namespace flang {
----------------
What is this for?


================
Comment at: flang/tools/flang-driver/driver.cpp:30
+extern int fc1_main(
+    llvm::ArrayRef<const char *> Argv, const char *Argv0, void *MainAddr);
+
----------------
Why isn't this declared in a header?


================
Comment at: flang/tools/flang-driver/fc1_main.cpp:32
+int fc1_main(
+    llvm::ArrayRef<const char *> Argv, const char *Argv0, void *MainAddr) {
+
----------------
MainAddr isn't used.


================
Comment at: flang/tools/flang-driver/fc1_main.cpp:54
+  // Execute the frontend actions.
+  { Success = ExecuteCompilerInvocation(Flang.get()); }
+
----------------
Why is this statement in a block? Is the result of CreateFromArgs supposed to affect the return value of this function?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86089