[PATCH] D88381: [Flang][Driver] Add PrintPreprocessed FrontendAction
sameeran joshi via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Sat Oct 17 10:51:59 PDT 2020
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
More information about the cfe-commits
mailing list