[PATCH] D88381: [Flang][Driver] Add PrintPreprocessed FrontendAction

[sameeran joshi via Phabricator via cfe-commits]

sameeranjoshi added a comment.

Thank you, this patch looks easy to understand as it's clearly separated from(`D87989`) the infrastructure changes needed for frontend actions.
A few comments inline.



================
Comment at: flang/include/flang/Frontend/CompilerInstance.h:85
+  /// Return parsing to be used by Actions.
+  Fortran::parser::Parsing &GetParsing() const { return *parsing_; }
+
----------------
If I am correct this seems to be an accessor member function and it should follow point 3 from flang style guide  mentioned at https://github.com/llvm/llvm-project/blob/master/flang/docs/C++style.md#naming

I am not aware if driver related patches follow llvm-project style.


================
Comment at: flang/include/flang/Frontend/CompilerInvocation.h:12
 #include "flang/Frontend/FrontendOptions.h"
+#include "flang/Parser/parsing.h"
 #include "clang/Basic/Diagnostic.h"
----------------
`Nit:` I believe `clang-format` is missing.


================
Comment at: flang/lib/Frontend/CMakeLists.txt:17
   FortranParser
+  FortranSemantics
   clangBasic
----------------
I believe this patch is a parsing and preprocessing related patch, is this used somewhere or should this be removed for now?


================
Comment at: flang/lib/Frontend/CompilerInstance.cpp:118
+  // Fortran::parser::option
+  // Set defaults for Parser, as it use as flang options
+  // Default consistent with the temporary driver in f18/f18.cpp
----------------
May be you meant `as it is used`?


================
Comment at: flang/lib/Frontend/FrontendAction.cpp:8
 //===----------------------------------------------------------------------===//
-
 #include "flang/Frontend/FrontendAction.h"
----------------
undo


================
Comment at: flang/lib/Frontend/FrontendAction.cpp:57
+  // Read files, scan and run preprocessor
+  // Needed by all next fases of the frontend
+  GetCompilerInstance().GetParsing().Prescan(path, options);
----------------
`Nit:` Converts to `phases` in english according to my browser. :)


================
Comment at: flang/lib/Frontend/FrontendActions.cpp:62
+    std::unique_ptr<llvm::raw_pwrite_stream> os;
+    os = ci.CreateDefaultOutputFile(
+        /*Binary=*/true, /*InFile=*/GetCurrentFileOrBufferName());
----------------
https://github.com/llvm/llvm-project/blob/master/flang/docs/C++style.md#c-language
point 4 
Can you use `braced initializers` ?

A more better version I think would be to simplify this to 
```
if (auto os{ci.CreateDefaultOutputFile(/*Binary=*/true, /*InFile=*/GetCurrentFileOrBufferName())}){
    (*os) << out.str();
} else {
  llvm::errs() << "Unable to create the output file\n";
  return;
}
```


================
Comment at: flang/test/Frontend/Inputs/hello-world.c:1
+a #include<stdio.h> int main() {
+  // printf() displays the string inside quotation
----------------
a - ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88381