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