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

Thu Oct 1 12:33:49 PDT 2020

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 `<filesystem>`

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 <file>">, MetaVarName<"<file>">;
----------------
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 <string>
 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<InputKind>(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 argument: '-o'
`flang-new -emit-obj` is still failing (which is expected) and we should have a test for it. Please revert this.

================
Comment at: flang/unittests/Frontend/CompilerInstanceTest.cpp:18
 using namespace Fortran::frontend;
+#include <filesystem>

----------------
Please use "llvm/Support/FileSystem.h" instead.

================
Comment at: flang/unittests/Frontend/CompilerInstanceTest.cpp:33
+  if (ec)
+    llvm::errs() << "Fail to create the file need by the test";
+  *(os) << inputSource;
----------------
Please use FAIL() instead: https://github.com/google/googletest/blob/master/googletest/docs/advanced.md#explicit-success-and-failure

================
Comment at: flang/unittests/Frontend/CompilerInstanceTest.cpp:43
+  CompilerInstance compInst;
+  Fortran::parser::AllSources &allSources{compInst.GetAllSources()};
+  const Fortran::parser::SourceFile *sf;
----------------
[nit] `allSources` is only used in one place - this variable is not needed

================
Comment at: flang/unittests/Frontend/CompilerInstanceTest.cpp:55
+  // 4. Delete file
+  llvm::sys::fs::remove(inputFilename);
+}
----------------
Forgot to test check the error code.

================
Comment at: flang/unittests/Frontend/InputOutputTest.cpp:18
+using namespace Fortran::frontend;
+#include <filesystem>
+
----------------
Please, use `"llvm/Support/FileSystem.h"` instead.

================
Comment at: flang/unittests/Frontend/InputOutputTest.cpp:22
+
+TEST(FrontendOutputTests, TestInputOutputStreamOwned) {
+  // 1. Prepare the input file to be used by IO
----------------
[nit] This is a test for `InputOutputTestAction`, perhaps:
```
TEST(FrontendAction, TestInputOutputStreamOwned)
```
?

================
Comment at: llvm/lib/Option/OptTable.cpp:642
     unsigned Flags = getInfo(Id).Flags;
+
     if (FlagsToInclude && !(Flags & FlagsToInclude))
----------------
Revert

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87989