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

[sameeran joshi via Phabricator via llvm-commits]

sameeranjoshi requested changes to this revision.
sameeranjoshi added a comment.

Thanks for working on it.
A few review comments/questions on changes in `flang` part from the patch.



================
Comment at: flang/include/flang/Frontend/CompilerInstance.h:93
+
+  static clang::IntrusiveRefCntPtr<clang::DiagnosticsEngine> createDiagnostics(
+      clang::DiagnosticOptions *Opts,
----------------
The block of comments above make sense for this function and not the currently mentioned one.
Please interchange/replace the comments to later declared function.
Wrong comments above could reflect in `doxygen APIs`, misleading the reader of code.


================
Comment at: flang/include/flang/FrontendTool/Utils.h:1
+
+//===--- Utils.h - Misc utilities for the flang front-end --------*- C++-*-===//
----------------
`nit:`: blank line.


================
Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:35
+  if (Flang->getFrontendOpts().ShowVersion) {
+    llvm::cl::PrintVersionMessage();
+    return true;
----------------
With 
```
clang --version
clang version 11.0.0 (https://github.com/llvm/llvm-project.git 1acf129bcf9a1b51e301a9fef151254ec4c7ec43)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/local/bin
```
whereas with 
```
./bin/flang-new --vesion
Flang experimental driver (flang-new)
```

I see both `clang` & `flang` call
`llvm::cl::PrintVersionMessage()` internally.

Is more information need to be registered in llvm(`llvm::cl`) for flang to give more detailed output or will that come later once we start adding more patch for driver?


================
Comment at: flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:39
+
+  return true;
+}
----------------
The comment in header for `ExecuteCompilerInvocation` mentions,
```
	/// \return - True on success.
	bool ExecuteCompilerInvocation(CompilerInstance *Flang);
```

Do we need to have a `false` somewhere here?

I see 2 scenarios when `ExecuteCompilerInvocation` might fail (there could definitely be more) and there we need an indication of failure by returning `false`,
1. When there is no actual execution of compiler.
2. The compilation in not successful.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86089