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