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

Mon Oct 19 10:24:30 PDT 2020

awarzynski added inline comments.

================
Comment at: flang/include/flang/Frontend/CompilerInstance.h:85
+  /// Return parsing to be used by Actions.
+  Fortran::parser::Parsing &GetParsing() const { return *parsing_; }
+
----------------
sameeranjoshi wrote:
> 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.
Thanks, good catch!

Flang driver is a subproject within Flang and we've assumed that it should follow Flang's coding style. I'm much more used to LLVM's coding style and also spend a lot of time in Clang - hence mistakes like this.

Personally I think that it's unfortunate that there are multiple coding styles within llvm-project. But this is neither time nor place to challenge that :)

Btw, sadly we've made similar mistake in other places: https://github.com/llvm/llvm-project/blob/master/flang/include/flang/Frontend/CompilerInstance.h#L30. I will refactor that in a separate patch.

================
Comment at: flang/include/flang/Frontend/CompilerInvocation.h:12
 #include "flang/Frontend/FrontendOptions.h"
+#include "flang/Parser/parsing.h"
 #include "clang/Basic/Diagnostic.h"
----------------
sameeranjoshi wrote:
> `Nit:` I believe `clang-format` is missing.
I did apply it and this line looks OK to me - what's missing?

================
Comment at: flang/include/flang/Frontend/CompilerInvocation.h:47-50
+  // Maps clang::driver options to frontend options use by
+  // Fortran::parser::Prescan
+  // TODO: map the option needed by the frontend
+  Fortran::parser::Options options_;
----------------
This comment should clearly communicate that these are _frontend_ options. `frontendOpts_` are _frontend_ driver options. The naming is perhaps a bit unfortunate :/

================
Comment at: flang/lib/Frontend/CMakeLists.txt:17
   FortranParser
+  FortranSemantics
   clangBasic
----------------
sameeranjoshi wrote:
> I believe this patch is a parsing and preprocessing related patch, is this used somewhere or should this be removed for now?
Not sure how that crept in - thank you!

================
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
----------------
sameeranjoshi wrote:
> May be you meant `as it is used`?
Good point :)

I have even more drastic suggestion - IMO the defaults should be set in a dedicated method. And the encoding for files could be set in the constructor of `CompilerInstance`.

I will implement that change. I will also add comments to highlight that we are using these defaults during the development. However, the end goal is to provide APIs for the user the set-up the frontend options.

================
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);
----------------
sameeranjoshi wrote:
> `Nit:` Converts to `phases` in english according to my browser. :)
Fixed :)

================
Comment at: flang/lib/Frontend/FrontendActions.cpp:62
+    std::unique_ptr<llvm::raw_pwrite_stream> os;
+    os = ci.CreateDefaultOutputFile(
+        /*Binary=*/true, /*InFile=*/GetCurrentFileOrBufferName());
----------------
sameeranjoshi wrote:
> 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;
> }
> ```
Neat suggestion, though: https://llvm.org/docs/CodingStandards.html#do-not-use-braced-initializer-lists-to-call-a-constructor. For consistency sake, I will follow the convention from Flang. I will also simplify this function a bit.

================
Comment at: flang/test/Frontend/print-preprocess-C-file.f90:8
+!--------------------------
+! TEST 1: Print to stdout (implicit)
+! RUN: not %flang-new -E %S/Inputs/hello-world.c  2>&1 | FileCheck %s
----------------
Probably copied from a different file - can be removed.

================
Comment at: flang/test/Frontend/print-preprocess-C-file.f90:10-11
+! RUN: not %flang-new -E %S/Inputs/hello-world.c  2>&1 | FileCheck %s
+! RUN: not %flang-new -E %S/Inputs/hello-world.c -o - 2>&1 | FileCheck %s
+! RUN: not %flang-new -E %S/Inputs/hello-world.c -o test.F90 2>&1 | FileCheck %s 
+
----------------
These 2 tests are no different from the previous one. The key thing to test here is that `flang-new -E` triggers an error for C files. I recommend removing that.

================
Comment at: flang/test/Frontend/print-preprocessed-file.f90:9-10
+! RUN: %flang-new -E %s  2>&1 | FileCheck %s
+! RUN: %flang-new -E  -o - %s 2>&1 | FileCheck %s
+! RUN: %flang-new -E -o %t  %s 2>&1 && FileCheck %s --input-file=%t
+
----------------
Similar comment as for print-preprocess-C-file.f90 (i.e. only one version is needed).

================
Comment at: flang/unittests/Frontend/PrintPreprocessedTest.cpp:9
+
+#include "flang/unittests/Frontend/PrintPreprocessedTest.cpp"
+#include "gtest/gtest.h"
----------------
Not sure how this crept in here. Need to delete.

================
Comment at: flang/unittests/Frontend/PrintPreprocessedTest.cpp:72
+  EXPECT_TRUE(!outputFileBuffer.empty());
+  EXPECT_TRUE(llvm::StringRef(outputFileBuffer.data()).equals("program b"));
+
----------------
This should be `startswith` instead of `equals`. The output buffer is not initialized and hence there will be garbage past "program b".`

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88381