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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits