awarzynski added a comment.
@CarolineConcatto , thank you for this patch! It implements some really
important functionality and IMO the overall structure is solid.
I've left quite a few comments, but mostly nits and suggestions for more
detailed comments. There's a few, but it's a relatively big patch :)
One suggestion - IMO this patch first and foremost creates the scaffolding for
writing frontend actions. The `test-IO` flag is only really there to facilitate
testing. Perhaps it's worth updating the commit message to reflect that? I
would definitely like to see a bit more emphasis on the fact that this patch
introduces the necessary APIs for reading and writing files. For me that's the
key functionality here.
Also, I tested this patch and currently `-test-IO` is listed together with
other `clang` options (check `clang --help-hidden`). Please, could you fix that?
================
Comment at: clang/include/clang/Driver/Options.td:3514
+//===----------------------------------------------------------------------===//
+def TEST_IO : Flag<["-"], "test-IO">, Flags<[HelpHidden, FlangOption,
FC1Option]>,Group<Action_Group>,
+HelpText<"Only run the InputOutputTest action">;
----------------
I think that `test-io` would be more consistent with other options in the file.
================
Comment at: clang/include/clang/Driver/Options.td:3515
+def TEST_IO : Flag<["-"], "test-IO">, Flags<[HelpHidden, FlangOption,
FC1Option]>,Group<Action_Group>,
+HelpText<"Only run the InputOutputTest action">;
+
----------------
I would help to make this a bit more specific. What about: `Run the
InputOuputTest action. Use for development and testing only`?
================
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:39-40
} else if (isa<CompileJobAction>(JA) || isa<BackendJobAction>(JA)) {
+ CmdArgs.push_back("-triple");
+ CmdArgs.push_back(Args.MakeArgString(TripleStr));
if (JA.getType() == types::TY_Nothing) {
----------------
It would be helpful to have a note that explains this change. Perhaps something
like this:
```
// TODO: Note that eventually all actions will require a triple (e.g. `-triple
aarch64-unknown-linux-gnu`). However, `-triple` is currently not supported by
`flang-new -fc1`, so we only add it to actions that we don't support/execute
just yet.
```
Probably above line 33. What do you think?
================
Comment at: clang/lib/Driver/Types.cpp:329
if (Driver.CCCIsCPP() || DAL.getLastArg(options::OPT_E) ||
+ DAL.getLastArg(options::OPT_TEST_IO) ||
DAL.getLastArg(options::OPT__SLASH_EP) ||
----------------
This should probably be the last option here (as it's the latest to be added).
Also, worth adding a comment about, e.g.:
```
-test-io is used by Flang to run InputOutputTest action
```
================
Comment at: flang/include/flang/Frontend/CompilerInstance.h:26
+ /// The Fortran file manager.
+ std::shared_ptr<Fortran::parser::AllSources> allSources_;
----------------
[nit] Forrtran or Flang?
================
Comment at: flang/include/flang/Frontend/CompilerInstance.h:33
+ /// Holds the name of the output file.
+ /// Helps to go through the list when working with the outputFiles_
+ struct OutputFile {
----------------
[nit] 'the outputFiles_' -> 'outputFiles_'
================
Comment at: flang/include/flang/Frontend/CompilerInstance.h:40-42
+ /// If the output doesn't support seeking (terminal, pipe), we switch
+ /// the stream to a buffer_ostream. These are the buffer and the original
+ /// stream.
----------------
[nit]
```
/// If the output doesn't support seeking (terminal, pipe), we switch
/// the stream to a buffer_ostream. These are the buffer and the original
/// stream.
```
vvv
```
/// Output stream that doesn't support seeking (e.g. terminal, pipe). This
stream is normally wrapped
/// in buffer_ostream before being passed to users (e.g. via CreateOutputFile).
```
================
Comment at: flang/include/flang/Frontend/CompilerInstance.h:48
+
+ /// Force an output buffer.
+ std::unique_ptr<llvm::raw_pwrite_stream> outputStream_;
----------------
I think that:
```
This field holds the output stream provided by the user. Normally, users of
CompilerInstance will call CreateOutputFile to obtain/create an output stream.
If they want to provide their own output stream, this field will facilitate
this. It is optional and will normally be just a nullptr.
```
would be a bit more informative :)
================
Comment at: flang/include/flang/Frontend/CompilerInstance.h:65
+ /// setInvocation - Replace the current invocation.
+ void SetInvocation(std::shared_ptr<CompilerInvocation> value);
----------------
[nit] Delete
================
Comment at: flang/include/flang/Frontend/CompilerInstance.h:68
+
+ /// Return the current source manager.
+ Fortran::parser::AllSources &GetAllSources() const { return *allSources_; }
----------------
[nit] There is no source manager here.
It's probably worth decorating everything related to `AllSources` with:
```
/// }
/// @name File manager
/// {
```
================
Comment at: flang/include/flang/Frontend/CompilerInstance.h:73
+
+ /// @name High-Level Operations
+ /// {
----------------
Missing `/// }`
================
Comment at: flang/include/flang/Frontend/CompilerInstance.h:76
+
+ /// ExecuteAction - Execute the provided action against the compiler's
+ /// CompilerInvocation object.
----------------
[nit] No need to repeat the name of the method/member variable in comments.
Same for the rest of the patch.
================
Comment at: flang/include/flang/Frontend/CompilerInstance.h:146-147
+
+ /// Has a function to create a new output file and add it to the list of
+ /// tracked output files.
+ ///
----------------
Remove `Has a`? Also, could you expand and clarify the differences between this
method and the one below? Ta!
================
Comment at: flang/include/flang/Frontend/CompilerInstance.h:158
+ /// \param error [out] - On failure, the error.
+ /// \param extension - The extension to use for derived output names.
+ /// \param binary - The mode to open the file in.
----------------
This argument is after `binary`
================
Comment at: flang/include/flang/Frontend/CompilerInstance.h:164
+ CreateOutputFile(llvm::StringRef outputPath, std::error_code &error,
+ bool binary, llvm::StringRef inFile,
+ llvm::StringRef extension, std::string *resultPathName);
----------------
This argument is not documented.
================
Comment at: flang/include/flang/Frontend/CompilerInstance.h:171-177
+ // OutputStream is private and its contenct cannot be checked outside
+ // CompilerInstance.
+ bool NullOutputStream() {
+ if (outputStream_ == nullptr)
+ return true;
+ return false;
+ }
----------------
[nit] I think that the comment is redundant - this is clear from the
implementation and it is a fairly standard C++ idiom (e.g. accessors).
It's a good practice to use proper questions to name functions like this one,
e.g. `IsOutputStreamNull`. Then, both possible return values answer this
question. Sorry, I couldn't find a link that would document this.
Also, this would be simpler, yet semantically equivalent:
```
bool IsOutputStreamNull() {return (outputStream_ == nullptr);}
```
================
Comment at: flang/include/flang/Frontend/FrontendAction.h:85
+
+ /// Set the source manager's main input file, and run the action.
+ llvm::Error Execute();
----------------
AFAIK, there is no source manager - only file manager.
================
Comment at: flang/include/flang/Frontend/FrontendOptions.h:19-21
+ // This is temporary solution
+ // Avoids Action = PrintPreprocessedInput as option 0
+ DummyAction = 0,
----------------
I recommend that this is replaced with:
```
InvalidAction = 0,
```
I think that this requires no comments.
================
Comment at: flang/include/flang/Frontend/FrontendOptions.h:111
+ const llvm::MemoryBuffer *GetBuffer() const {
+ assert(IsBuffer());
+ return buffer_;
----------------
Add message, e.g.
```
assert(IsBuffer() && "Requested buffer_, but it is empty!");
```
================
Comment at: flang/lib/Frontend/CompilerInstance.cpp:26
+CompilerInstance::~CompilerInstance() {
+ assert(OutputFiles.empty() && "Still output files in flight?");
+}
----------------
`outputFiles_`?
================
Comment at: flang/lib/Frontend/CompilerInstance.cpp:94-100
+ // Flushes the stream
+ if (!binary || os->supportsSeeking())
+ return std::move(os);
+ auto b = std::make_unique<llvm::buffer_ostream>(*os);
+ assert(!nonSeekStream_);
+ nonSeekStream_ = std::move(os);
+
----------------
The stream is not flushed until it's destructor is called. This happens
elsewhere. Here we just return the stream corresponding to the output file.
Since there's a distinction between seekable and non-seekable streams, I'd be
tempted to split this as follows:
```
// Return the stream corresponding to the output file. For non-seekable
streams, wrap it in llvm::buffer_ostream first.
if (!binary || os->supportsSeeking())
return std::move(os);
assert(!nonSeekStream_ && "The non-seek stream has already been set!");
auto b = std::make_unique<llvm::buffer_ostream>(*os);
nonSeekStream_ = std::move(os);
return std::move(b);
```
================
Comment at: flang/lib/Frontend/FrontendActions.cpp:19
+
+ // Get the name of the file from FrontendInputFile current
+ std::string path{GetCurrentFileOrBufferName()};
----------------
[nit] Get the name of the current file from FrontendInputFile
================
Comment at: flang/test/Flang-Driver/driver-help-hidden.f90:7
+! RUN: %flang-new --help-hidden 2>&1 | FileCheck %s
+! RUN: not %flang-new -help-hidden 2>&1 | FileCheck %s --check-prefix=ERROR
+
----------------
[nit] Both `--check-prefix=ERROR` and `--check-prefix=ERROR-HIDDEN` check the
`help-hidden` flang`. Perhaps `--check-prefix=ERROR-FLANG-NEW` and
`--check-prefix=ERROR-FLANG-NEW-FC1` would make the difference between the two
cases more obvious?
================
Comment at: flang/test/Flang-Driver/driver-help-hidden.f90:27
+!-------------------------------------------------------------
+! EXPECTED OUTPUT FOR FLANG FRONTEND DRIVER (flang-new -fc1)
+!-------------------------------------------------------------
----------------
This is the expected error for both `flang-new` (compiler driver) and
`flang-new -fc1` (the frontend driver).
================
Comment at: flang/test/Flang-Driver/emit-obj.f90:2
! RUN: not %flang-new %s 2>&1 | FileCheck %s --check-prefix=ERROR-IMPLICIT
-! RUN: not %flang-new -emit-obj %s 2>&1 | FileCheck %s
--check-prefix=ERROR-EXPLICIT
! RUN: not %flang-new -fc1 -emit-obj %s 2>&1 | FileCheck %s
--check-prefix=ERROR-FC1
----------------
Why is this line deleted? `flang-new -emit-obj` should still fail, right?
================
Comment at: flang/test/Flang-Driver/emit-obj.f90:12
! ERROR-IMPLICIT: error: unknown argument: '-emit-obj'
-! ERROR-IMPLICIT: error: unknown argument: '-o'
-
-! ERROR-EXPLICIT: error: unknown argument: '-o'
+! ERROR-IMPLICIT-NOT: error: unknown argument: '-o'
----------------
I'd be tempted just to delete this line - we have tests elsewhere to show that
`-o` is indeed supported.
================
Comment at: flang/test/Frontend/multiple-input-files.f90:1
+! RUN: rm -rf %S/multiple-input-files.txt %S/Inputs/empty-file.txt
+
----------------
I think that this test will be much more interesting and useful if
`empty-file.f90` is not empty :)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87989/new/
https://reviews.llvm.org/D87989
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
