[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thank you all for your input! Before merging I took the liberty to rename 
`NoClangOption` as `FlangOnlyOption`. The new name reflects better what the 
flag is introduced for. Also, based on responses to [1], it is unlikely that it 
will be used beyond flang-only options. If that changes we can always rename it.

[1] http://lists.llvm.org/pipermail/cfe-dev/2020-October/066953.html


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

[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O

[Andrzej Warzynski via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG4c5906cffd04: [Flang][Driver] Add infrastructure for basic 
frontend actions and file I/O (authored by CarolineConcatto, committed by 
awarzynski).

Changed prior to commit:
  https://reviews.llvm.org/D87989?vs=299033=300485#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87989

Files:
  clang/include/clang/Driver/Options.h
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/lib/Driver/Types.cpp
  clang/test/Driver/immediate-options.c
  flang/include/flang/Frontend/CompilerInstance.h
  flang/include/flang/Frontend/CompilerInvocation.h
  flang/include/flang/Frontend/FrontendAction.h
  flang/include/flang/Frontend/FrontendActions.h
  flang/include/flang/Frontend/FrontendOptions.h
  flang/include/flang/FrontendTool/Utils.h
  flang/lib/Frontend/CMakeLists.txt
  flang/lib/Frontend/CompilerInstance.cpp
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendAction.cpp
  flang/lib/Frontend/FrontendActions.cpp
  flang/lib/Frontend/FrontendOptions.cpp
  flang/lib/FrontendTool/CMakeLists.txt
  flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
  flang/test/Flang-Driver/driver-help-hidden.f90
  flang/test/Flang-Driver/driver-help.f90
  flang/test/Flang-Driver/emit-obj.f90
  flang/test/Frontend/Inputs/hello-world.f90
  flang/test/Frontend/input-output-file.f90
  flang/test/Frontend/multiple-input-files.f90
  flang/test/lit.cfg.py
  flang/tools/flang-driver/fc1_main.cpp
  flang/unittests/Frontend/CMakeLists.txt
  flang/unittests/Frontend/CompilerInstanceTest.cpp
  flang/unittests/Frontend/InputOutputTest.cpp

Index: flang/unittests/Frontend/InputOutputTest.cpp
===
--- /dev/null
+++ flang/unittests/Frontend/InputOutputTest.cpp
@@ -0,0 +1,76 @@
+//===- unittests/Frontend/OutputStreamTest.cpp --- FrontendAction tests --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "gtest/gtest.h"
+#include "flang/Frontend/CompilerInstance.h"
+#include "flang/Frontend/CompilerInvocation.h"
+#include "flang/Frontend/FrontendOptions.h"
+#include "flang/FrontendTool/Utils.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/raw_ostream.h"
+
+using namespace Fortran::frontend;
+
+namespace {
+
+TEST(FrontendAction, TestInputOutputTestAction) {
+  std::string inputFile = "io-file-test.f";
+  std::error_code ec;
+
+  // 1. Create the input file for the file manager
+  // AllSources (which is used to manage files inside every compiler instance),
+  // works with paths. This means that it requires a physical file. Create one.
+  std::unique_ptr os{
+  new llvm::raw_fd_ostream(inputFile, ec, llvm::sys::fs::OF_None)};
+  if (ec)
+FAIL() << "Failed to create the input file";
+
+  // Populate the input file with the pre-defined input and flush it.
+  *(os) << "End Program arithmetic";
+  os.reset();
+
+  // Get the path of the input file
+  llvm::SmallString<64> cwd;
+  if (std::error_code ec = llvm::sys::fs::current_path(cwd))
+FAIL() << "Failed to obtain the current working directory";
+  std::string testFilePath(cwd.c_str());
+  testFilePath += "/" + inputFile;
+
+  // 2. Prepare the compiler (CompilerInvocation + CompilerInstance)
+  CompilerInstance compInst;
+  compInst.CreateDiagnostics();
+  auto invocation = std::make_shared();
+  invocation->GetFrontendOpts().programAction_ = InputOutputTest;
+  compInst.SetInvocation(std::move(invocation));
+  compInst.GetFrontendOpts().inputs_.push_back(
+  FrontendInputFile(/*File=*/testFilePath, Language::Fortran));
+
+  // 3. Set-up the output stream. Using output buffer wrapped as an output
+  // stream, as opposed to an actual file (or a file descriptor).
+  llvm::SmallVector outputFileBuffer;
+  std::unique_ptr outputFileStream(
+  new llvm::raw_svector_ostream(outputFileBuffer));
+  compInst.SetOutputStream(std::move(outputFileStream));
+
+  // 4. Run the earlier defined FrontendAction
+  bool success = ExecuteCompilerInvocation();
+
+  EXPECT_TRUE(success);
+  EXPECT_TRUE(!outputFileBuffer.empty());
+  EXPECT_TRUE(llvm::StringRef(outputFileBuffer.data())
+  .startswith("End Program arithmetic"));
+
+  // 5. Clear the input and the output files. Since we used an output buffer,
+  // there are no physical output files to delete.
+  ec = llvm::sys::fs::remove(inputFile);
+  if (ec)
+FAIL() << "Failed to delete the test file";
+
+  compInst.ClearOutputFiles(/*EraseFiles=*/false);
+}
+} // namespace
Index: 

[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O

[Richard Barton via Phabricator via cfe-commits]

richard.barton.arm accepted this revision.
richard.barton.arm added a comment.

I'm happy to accept this revision based on @SouraVX 's code review and the fact 
that Andrzej has sent multiple RFCs covering the clang-side changes, including 
the Options flags (latest here 
http://lists.llvm.org/pipermail/cfe-dev/2020-October/067079.html). I think this 
code is good enough to commit.

Thanks for the code and reviews!


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

[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O

[Sourabh Singh Tomar via Phabricator via cfe-commits]

SouraVX accepted this revision.
SouraVX added a comment.
This revision is now accepted and ready to land.

Thanks for your patience. LGTM! at least the part I reviewed. Though I would 
vote for having another approval(From some senior folks in community)


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

[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Ping


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

[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski updated this revision to Diff 299033.
awarzynski added a comment.

Simplify the API for creating output files

The originally implemented API was overly complicated and not yet required.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87989

Files:
  clang/include/clang/Driver/Options.h
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/lib/Driver/Types.cpp
  clang/test/Driver/immediate-options.c
  flang/include/flang/Frontend/CompilerInstance.h
  flang/include/flang/Frontend/CompilerInvocation.h
  flang/include/flang/Frontend/FrontendAction.h
  flang/include/flang/Frontend/FrontendActions.h
  flang/include/flang/Frontend/FrontendOptions.h
  flang/include/flang/FrontendTool/Utils.h
  flang/lib/Frontend/CMakeLists.txt
  flang/lib/Frontend/CompilerInstance.cpp
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendAction.cpp
  flang/lib/Frontend/FrontendActions.cpp
  flang/lib/Frontend/FrontendOptions.cpp
  flang/lib/FrontendTool/CMakeLists.txt
  flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
  flang/test/Flang-Driver/driver-help-hidden.f90
  flang/test/Flang-Driver/driver-help.f90
  flang/test/Flang-Driver/emit-obj.f90
  flang/test/Frontend/Inputs/hello-world.f90
  flang/test/Frontend/input-output-file.f90
  flang/test/Frontend/multiple-input-files.f90
  flang/test/lit.cfg.py
  flang/tools/flang-driver/fc1_main.cpp
  flang/unittests/Frontend/CMakeLists.txt
  flang/unittests/Frontend/CompilerInstanceTest.cpp
  flang/unittests/Frontend/InputOutputTest.cpp

Index: flang/unittests/Frontend/InputOutputTest.cpp
===
--- /dev/null
+++ flang/unittests/Frontend/InputOutputTest.cpp
@@ -0,0 +1,72 @@
+//===- unittests/Frontend/OutputStreamTest.cpp --- FrontendAction tests --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "gtest/gtest.h"
+#include "flang/Frontend/CompilerInstance.h"
+#include "flang/Frontend/CompilerInvocation.h"
+#include "flang/Frontend/FrontendOptions.h"
+#include "flang/FrontendTool/Utils.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/raw_ostream.h"
+
+using namespace Fortran::frontend;
+
+namespace {
+
+TEST(FrontendAction, TestInputOutputTestAction) {
+  std::string inputFile = "io-file-test.f";
+  std::error_code ec;
+
+  // 1. Create the input file for the file manager
+  // AllSources (which is used to manage files inside every compiler instance),
+  // works with paths. This means that it requires a physical file. Create one.
+  std::unique_ptr os{
+  new llvm::raw_fd_ostream(inputFile, ec, llvm::sys::fs::OF_None)};
+  if (ec)
+FAIL() << "Failed to create the input file";
+
+  // Populate the input file with the pre-defined input and flush it.
+  *(os) << "End Program arithmetic";
+  os.reset();
+
+  // Get the path of the input file
+  llvm::SmallString<64> cwd;
+  if (std::error_code ec = llvm::sys::fs::current_path(cwd))
+FAIL() << "Failed to obtain the current working directory";
+  std::string testFilePath(cwd.c_str());
+  testFilePath += "/" + inputFile;
+
+  // 2. Prepare the compiler (CompilerInvocation + CompilerInstance)
+  CompilerInstance compInst;
+  compInst.CreateDiagnostics();
+  auto invocation = std::make_shared();
+  invocation->GetFrontendOpts().programAction_ = InputOutputTest;
+  compInst.SetInvocation(std::move(invocation));
+  compInst.GetFrontendOpts().inputs_.push_back(
+  FrontendInputFile(/*File=*/testFilePath, Language::Fortran));
+
+  // 3. Set-up the output stream. Using output buffer wrapped as an output
+  // stream, as opposed to an actual file (or a file descriptor).
+  llvm::SmallVector outputFileBuffer;
+  std::unique_ptr outputFileStream(
+  new llvm::raw_svector_ostream(outputFileBuffer));
+  compInst.SetOutputStream(std::move(outputFileStream));
+
+  // 4. Run the earlier defined FrontendAction
+  bool success = ExecuteCompilerInvocation();
+
+  EXPECT_TRUE(success);
+  EXPECT_TRUE(!outputFileBuffer.empty());
+  EXPECT_TRUE(llvm::StringRef(outputFileBuffer.data())
+  .startswith("End Program arithmetic"));
+
+  // 5. Clear the output files. Since we used an output buffer, there are no
+  // physical files to delete.
+  compInst.ClearOutputFiles(/*EraseFiles=*/false);
+}
+} // namespace
Index: flang/unittests/Frontend/CompilerInstanceTest.cpp
===
--- flang/unittests/Frontend/CompilerInstanceTest.cpp
+++ flang/unittests/Frontend/CompilerInstanceTest.cpp
@@ -9,6 +9,7 @@
 #include "flang/Frontend/CompilerInstance.h"
 #include 

[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

I've added more reviewers for the Clang side of this patch. I choose people who 
most recently changed the functions/files that this patch modifies. Any input 
much appreciated! For more context regarding Clang changes: 
http://lists.llvm.org/pipermail/cfe-dev/2020-October/066953.html


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

[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski commandeered this revision.
awarzynski edited reviewers, added: CarolineConcatto; removed: awarzynski.
awarzynski added a comment.

Thank you for reviewing @SouraVX! I'm just about to submit an updated patch 
with the requested changes.

@CarolineConcatto has recently moved to a different project and so it will be 
mostly me updating this. @CarolineConcatto , thanks for all the effort!




Comment at: clang/include/clang/Driver/Options.td:63
 
+// ClangOption - This option should not be accepted by Clang.
+def NoClangOption : OptionFlag;

SouraVX wrote:
>  `NoClangOption` ? Is this a Typo, or am I missing the intent behind this ?
Yup, a typo, thanks!



Comment at: flang/include/flang/Frontend/CompilerInstance.h:136
+  /// Add an output file onto the list of tracked output files.
+  ///
+  /// \param outFile - The output file info.

SouraVX wrote:
> NIT: Blank line ?
That's the convention for Doxygen, isn't it?



Comment at: flang/lib/Frontend/CompilerInstance.cpp:67
+  // Create the name of the output file
+  if (!outputPath.empty()) {
+outFile = std::string(outputPath);

SouraVX wrote:
> Can this be simplified ? Maybe a switch case ?
Switch statement would be tricky, but I agree that this is unnecessarily 
complex.


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

[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski updated this revision to Diff 298407.
awarzynski added a comment.

Address PR comments, clang-format, rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87989

Files:
  clang/include/clang/Driver/Options.h
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/lib/Driver/Types.cpp
  clang/test/Driver/immediate-options.c
  flang/include/flang/Frontend/CompilerInstance.h
  flang/include/flang/Frontend/CompilerInvocation.h
  flang/include/flang/Frontend/FrontendAction.h
  flang/include/flang/Frontend/FrontendActions.h
  flang/include/flang/Frontend/FrontendOptions.h
  flang/include/flang/FrontendTool/Utils.h
  flang/lib/Frontend/CMakeLists.txt
  flang/lib/Frontend/CompilerInstance.cpp
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendAction.cpp
  flang/lib/Frontend/FrontendActions.cpp
  flang/lib/Frontend/FrontendOptions.cpp
  flang/lib/FrontendTool/CMakeLists.txt
  flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
  flang/test/Flang-Driver/driver-help-hidden.f90
  flang/test/Flang-Driver/driver-help.f90
  flang/test/Flang-Driver/emit-obj.f90
  flang/test/Frontend/Inputs/hello-world.f90
  flang/test/Frontend/input-output-file.f90
  flang/test/Frontend/multiple-input-files.f90
  flang/test/lit.cfg.py
  flang/tools/flang-driver/fc1_main.cpp
  flang/unittests/Frontend/CMakeLists.txt
  flang/unittests/Frontend/CompilerInstanceTest.cpp
  flang/unittests/Frontend/InputOutputTest.cpp

Index: flang/unittests/Frontend/InputOutputTest.cpp
===
--- /dev/null
+++ flang/unittests/Frontend/InputOutputTest.cpp
@@ -0,0 +1,72 @@
+//===- unittests/Frontend/OutputStreamTest.cpp --- FrontendAction tests --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "gtest/gtest.h"
+#include "flang/Frontend/CompilerInstance.h"
+#include "flang/Frontend/CompilerInvocation.h"
+#include "flang/Frontend/FrontendOptions.h"
+#include "flang/FrontendTool/Utils.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/raw_ostream.h"
+
+using namespace Fortran::frontend;
+
+namespace {
+
+TEST(FrontendAction, TestInputOutputTestAction) {
+  std::string inputFile = "io-file-test.f";
+  std::error_code ec;
+
+  // 1. Create the input file for the file manager
+  // AllSources (which is used to manage files inside every compiler instance),
+  // works with paths. This means that it requires a physical file. Create one.
+  std::unique_ptr os{
+  new llvm::raw_fd_ostream(inputFile, ec, llvm::sys::fs::OF_None)};
+  if (ec)
+FAIL() << "Failed to create the input file";
+
+  // Populate the input file with the pre-defined input and flush it.
+  *(os) << "End Program arithmetic";
+  os.reset();
+
+  // Get the path of the input file
+  llvm::SmallString<64> cwd;
+  if (std::error_code ec = llvm::sys::fs::current_path(cwd))
+FAIL() << "Failed to obtain the current working directory";
+  std::string testFilePath(cwd.c_str());
+  testFilePath += "/" + inputFile;
+
+  // 2. Prepare the compiler (CompilerInvocation + CompilerInstance)
+  CompilerInstance compInst;
+  compInst.CreateDiagnostics();
+  auto invocation = std::make_shared();
+  invocation->GetFrontendOpts().programAction_ = InputOutputTest;
+  compInst.SetInvocation(std::move(invocation));
+  compInst.GetFrontendOpts().inputs_.push_back(
+  FrontendInputFile(/*File=*/testFilePath, Language::Fortran));
+
+  // 3. Set-up the output stream. Using output buffer wrapped as an output
+  // stream, as opposed to an actual file (or a file descriptor).
+  llvm::SmallVector outputFileBuffer;
+  std::unique_ptr outputFileStream(
+  new llvm::raw_svector_ostream(outputFileBuffer));
+  compInst.SetOutputStream(std::move(outputFileStream));
+
+  // 4. Run the earlier defined FrontendAction
+  bool success = ExecuteCompilerInvocation();
+
+  EXPECT_TRUE(success);
+  EXPECT_TRUE(!outputFileBuffer.empty());
+  EXPECT_TRUE(llvm::StringRef(outputFileBuffer.data())
+  .startswith("End Program arithmetic"));
+
+  // 5. Clear the output files. Since we used an output buffer, there are no
+  // physical files to delete.
+  compInst.ClearOutputFiles(/*EraseFiles=*/false);
+}
+} // namespace
Index: flang/unittests/Frontend/CompilerInstanceTest.cpp
===
--- flang/unittests/Frontend/CompilerInstanceTest.cpp
+++ flang/unittests/Frontend/CompilerInstanceTest.cpp
@@ -9,6 +9,7 @@
 #include "flang/Frontend/CompilerInstance.h"
 #include "flang/Frontend/TextDiagnosticPrinter.h"
 #include "clang/Basic/DiagnosticOptions.h"

[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O

[Sourabh Singh Tomar via Phabricator via cfe-commits]

SouraVX added a comment.

Thanks for the patch! Overall it's in pretty nice state and conformant to 
design highlighted. 
Could you please `clang-format` this patch, There are lot of `lint` warning 
that causes lot of noise while reviewing.
Couple of NIT comments  inline. I've tried marking all but, Could you please 
finish comments with period.

I think it would good if someone from `clang` community can also take a brief 
look.




Comment at: clang/include/clang/Driver/Options.td:63
 
+// ClangOption - This option should not be accepted by Clang.
+def NoClangOption : OptionFlag;

 `NoClangOption` ? Is this a Typo, or am I missing the intent behind this ?



Comment at: clang/lib/Driver/Driver.cpp:1579
 
-  if (IsFlangMode())
+  if (IsFlangMode()) {
 IncludedFlagsBitmask |= options::FlangOption;

NIT: May choose to avoid trivial braces. 
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements



Comment at: flang/include/flang/Frontend/CompilerInstance.h:136
+  /// Add an output file onto the list of tracked output files.
+  ///
+  /// \param outFile - The output file info.

NIT: Blank line ?



Comment at: flang/include/flang/Frontend/CompilerInstance.h:230
+
+  // Allow the frontend compiler to write in the output stream
+  void WriteOutputStream(const std::string ) {

NIT: Please finish the comment with a period.



Comment at: flang/lib/Frontend/CompilerInstance.cpp:66
+
+  // Create the name of the output file
+  if (!outputPath.empty()) {

NIT: Missing period at the end ?



Comment at: flang/lib/Frontend/CompilerInstance.cpp:67
+  // Create the name of the output file
+  if (!outputPath.empty()) {
+outFile = std::string(outputPath);

Can this be simplified ? Maybe a switch case ?



Comment at: flang/lib/Frontend/FrontendActions.cpp:25
+
+  // Set/store input file info into compiler instace
+  CompilerInstance  = GetCompilerInstance();

NIT: Instance ? and period at end ?



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

[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

@reviewers A note regarding the changes in Clang.

This patch introduces a Flang option (`-test-io`), that should not be 
available/visible in Clang. AFAIK, there's no precedent of that, hence 
`options::NoClangOption` is introduced. This is discussed in more detail here: 
http://lists.llvm.org/pipermail/cfe-dev/2020-October/066953.html.


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

[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

LGTM, thanks for working on this!

As this is a fairly large change, could you wait for one more reviewer to 
approve? Thanks!


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

[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O

[Caroline via Phabricator via cfe-commits]

CarolineConcatto marked 7 inline comments as done.
CarolineConcatto added a comment.

@awarzynski patch updated.


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

[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O

[Caroline via Phabricator via cfe-commits]

CarolineConcatto updated this revision to Diff 296066.
CarolineConcatto edited the summary of this revision.
CarolineConcatto added a comment.

address reviews comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87989

Files:
  clang/include/clang/Driver/Options.h
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/Driver.cpp
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/lib/Driver/Types.cpp
  clang/test/Driver/immediate-options.c
  flang/include/flang/Frontend/CompilerInstance.h
  flang/include/flang/Frontend/CompilerInvocation.h
  flang/include/flang/Frontend/FrontendAction.h
  flang/include/flang/Frontend/FrontendActions.h
  flang/include/flang/Frontend/FrontendOptions.h
  flang/include/flang/FrontendTool/Utils.h
  flang/lib/Frontend/CMakeLists.txt
  flang/lib/Frontend/CompilerInstance.cpp
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendAction.cpp
  flang/lib/Frontend/FrontendActions.cpp
  flang/lib/Frontend/FrontendOptions.cpp
  flang/lib/FrontendTool/CMakeLists.txt
  flang/lib/FrontendTool/ExecuteCompilerInvocation.cpp
  flang/test/Flang-Driver/driver-help-hidden.f90
  flang/test/Flang-Driver/driver-help.f90
  flang/test/Flang-Driver/emit-obj.f90
  flang/test/Frontend/Inputs/hello-world.f90
  flang/test/Frontend/input-output-file.f90
  flang/test/Frontend/multiple-input-files.f90
  flang/test/lit.cfg.py
  flang/tools/flang-driver/fc1_main.cpp
  flang/unittests/Frontend/CMakeLists.txt
  flang/unittests/Frontend/CompilerInstanceTest.cpp
  flang/unittests/Frontend/InputOutputTest.cpp
  llvm/include/llvm/Option/OptTable.h

Index: llvm/include/llvm/Option/OptTable.h
===
--- llvm/include/llvm/Option/OptTable.h
+++ llvm/include/llvm/Option/OptTable.h
@@ -243,7 +243,8 @@
   /// \param Usage - USAGE: Usage
   /// \param Title - OVERVIEW: Title
   /// \param FlagsToInclude - If non-zero, only include options with any
-  /// of these flags set.
+  /// of these flags set. Takes precedence over
+  /// FlagsToExclude.
   /// \param FlagsToExclude - Exclude options with any of these flags set.
   /// \param ShowAllAliases - If true, display all options including aliases
   /// that don't have help texts. By default, we display
Index: flang/unittests/Frontend/InputOutputTest.cpp
===
--- /dev/null
+++ flang/unittests/Frontend/InputOutputTest.cpp
@@ -0,0 +1,72 @@
+//===- unittests/Frontend/OutputStreamTest.cpp --- FrontendAction tests --===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "flang/Frontend/CompilerInstance.h"
+#include "flang/Frontend/CompilerInvocation.h"
+#include "flang/Frontend/FrontendOptions.h"
+#include "flang/FrontendTool/Utils.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/raw_ostream.h"
+#include "gtest/gtest.h"
+
+using namespace Fortran::frontend;
+
+namespace {
+
+TEST(FrontendAction, TestInputOutputTestAction) {
+  std::string inputFile = "io-file-test.f";
+  std::error_code ec;
+
+  // 1. Create the input file for the file manager
+  // AllSources (which is used to manage files inside every compiler instance),
+  // works with paths. This means that it requires a physical file. Create one.
+  std::unique_ptr os{
+  new llvm::raw_fd_ostream(inputFile, ec, llvm::sys::fs::OF_None)};
+  if (ec)
+FAIL() << "Failed to create the input file";
+
+  // Populate the input file with the pre-defined input and flush it.
+  *(os) << "End Program arithmetic";
+  os.reset();
+
+  // Get the path of the input file
+  llvm::SmallString<64> cwd;
+  if (std::error_code ec = llvm::sys::fs::current_path(cwd))
+FAIL() << "Failed to obtain the current working directory";
+  std::string testFilePath(cwd.c_str());
+  testFilePath += "/" + inputFile;
+
+  // 2. Prepare the compiler (CompilerInvocation + CompilerInstance)
+  CompilerInstance compInst;
+  compInst.CreateDiagnostics();
+  auto invocation = std::make_shared();
+  invocation->GetFrontendOpts().programAction_ = InputOutputTest;
+  compInst.SetInvocation(std::move(invocation));
+  compInst.GetFrontendOpts().inputs_.push_back(
+  FrontendInputFile(/*File=*/testFilePath, Language::Fortran));
+
+  // 3. Set-up the output stream. Using output buffer wrapped as an output
+  // stream, as opposed to an actual file (or a file descriptor).
+  llvm::SmallVector outputFileBuffer;
+  std::unique_ptr outputFileStream(
+  new llvm::raw_svector_ostream(outputFileBuffer));
+  compInst.SetOutputStream(std::move(outputFileStream));
+
+  // 

[PATCH] D87989: [Flang][Driver] Add infrastructure for basic frontend actions and file I/O

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

@CarolineConcatto thank you again for working on this! The structure is good, 
but IMHO this patch could be polished a bit more. Overall:

- could you make sure that this patch does not change the output from `clang 
-help`?
- doxygen comments are consistent
- unittests are a bit better documentated (what and why is tested?)
- use `llvm/Support/FileSystem.h`  instead of ``

More comments inline. Otherwise, I think that this is almost ready :)




Comment at: clang/include/clang/Driver/Options.td:2795
 def object : Flag<["-"], "object">;
-def o : JoinedOrSeparate<["-"], "o">, Flags<[DriverOption, RenderAsInput, 
CC1Option, CC1AsOption]>,
+def o : JoinedOrSeparate<["-"], "o">, Flags<[DriverOption, RenderAsInput, 
CC1Option, CC1AsOption, FC1Option]>,
   HelpText<"Write output to ">, MetaVarName<"">;

What about FlangOption?



Comment at: clang/lib/Driver/Driver.cpp:1576-1579
+  } else {
+ExcludedFlagsBitmask |= options::FlangOption;
+ExcludedFlagsBitmask |= options::FC1Option;
+  }

With this, the following is empty:
```
$ bin/clang --help | grep help
```

This is what I'd expect instead (i.e. the behavior shouldn't change):
```
$ bin/clang --help | grep help
  --help-hidden   Display help for hidden options
  -help   Display available options
```

This needs a bit more polishing.



Comment at: clang/test/Driver/immediate-options.c:5
 // HELP-NOT: driver-mode
+// HEKP-NOT: test-io
 

HELP instead of HEKP

Could add a comment why this particular test is here?



Comment at: flang/include/flang/Frontend/CompilerInstance.h:58
+
+  /// }
+  /// @name Compiler Invocation

I can't see a matching `/// }` for this. DELETEME



Comment at: flang/include/flang/Frontend/CompilerInstance.h:85
+  /// CompilerInvocation object.
+  /// Note that this routine may write output to 'stderr'.
+  /// \param act - The action to execute.

From what I can see, this is not the case. Could you update?



Comment at: flang/include/flang/Frontend/FrontendAction.h:23
+protected:
+  /// @}
+  /// @name Implementation Action Interface

DELETEME (missing matching `/// {`)



Comment at: flang/include/flang/Frontend/FrontendAction.h:42
+
+  /// @}
+  /// @name Compiler Instance Access

I think that this should be on line 38.



Comment at: flang/include/flang/Frontend/FrontendOptions.h:16
 #include 
 namespace Fortran::frontend {
 

[nit] Empty line



Comment at: flang/include/flang/Frontend/FrontendOptions.h:19
+enum ActionKind {
+  // This is temporary solution
+  // Avoids Action = InputOutputTest as option 0

Temporary solution to what?



Comment at: flang/include/flang/Frontend/FrontendOptions.h:56-59
+  Fortran77,
+  Fortran90,
+  Fortran95,
+  FortranF18

Could we defer adding this to later? This patch is already rather larger.



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:72
 dashX = llvm::StringSwitch(XValue)
-.Case("f90", Language::Fortran)
+.Case("f90", Language::Fortran90)
 .Default(Language::Unknown);

Please, could we defer this to later?



Comment at: flang/lib/Frontend/FrontendOptions.cpp:18-23
+  .Cases("ff", "fpp", "FPP", "cuf", "CUF", "fir", "FOR", "for",
+ Language::Fortran)
+  .Cases("f", "F", "f77", Language::Fortran77)
+  .Cases("f90", "F90", ".ff90", Language::Fortran90)
+  .Cases("f95", "F95", "ff95", Language::Fortran95)
+  .Cases("f18", "F18", Language::FortranF18)

Could you reduce it to `Language::Fortran` only? We can deal with various 
variants in a separate patch when it becomes relevant.



Comment at: flang/test/Flang-Driver/driver-help.f90:14
+
 ! REQUIRES: new-flang-driver
 

Probably should be at the top (for consistency with driver-help-hidden.f90)



Comment at: flang/test/Flang-Driver/driver-help.f90:22
+! CHECK-FLANG-NEXT:OPTIONS:
+! CHECK-FLANG-NEXT: -help Display available options
+! CHECK-FLANG-NEXT: --version Print version information

What about `-o`?



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

CarolineConcatto wrote:
> awarzynski wrote:
> > Why is this line deleted? `flang-new -emit-obj` should still fail, right?
> I remove this because the text does not apply any more
> ! ERROR-IMPLICIT: error: unknown