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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
