awarzynski added a comment. Hi @arnamoy10, thank you for working on this! Overall this looks good to me, but I'm wondering whether we could simplify it a bit? More comments inline.
================ Comment at: clang/include/clang/Driver/Options.td:4163 defm d_lines_as_comments : BooleanFFlag<"d-lines-as-comments">, Group<gfortran_Group>; -defm default_double_8 : BooleanFFlag<"default-double-8">, Group<gfortran_Group>; -defm default_integer_8 : BooleanFFlag<"default-integer-8">, Group<gfortran_Group>; -defm default_real_8 : BooleanFFlag<"default-real-8">, Group<gfortran_Group>; +defm default_double_8 : BooleanFFlag<"default-double-8">, Group<gfortran_Group>, Flags<[FlangOption,FC1Option]>; +defm default_integer_8 : BooleanFFlag<"default-integer-8">, Group<gfortran_Group>, Flags<[FlangOption,FC1Option]>; ---------------- Could you add a help text? Note that once you add a help text, you will have to add `FlangOnlyOption` flag to make sure this option doesn't show up in: ``` clang -help ``` Some for other options here. Also, at that point, I suggest moving these options near other Flang options: https://github.com/llvm/llvm-project/blob/ec4fb5bcd3b92867156a5bd75fa0be4c74084f3c/clang/include/clang/Driver/Options.td#L4224-L4238. AFAIK, these options are no longer forwarded to `gfortran` anyway (since this [[ https://github.com/llvm/llvm-project/commit/6a75496836ea14bcfd2f4b59d35a1cad4ac58cee | patch ]]). ================ Comment at: clang/include/clang/Driver/Options.td:4166 +defm default_real_8 : BooleanFFlag<"default-real-8">, Group<gfortran_Group>, Flags<[FlangOption,FC1Option]>; +defm large_sizes : BooleanFFlag<"large-sizes">, Group<gfortran_Group>, Flags<[FlangOption,FC1Option]>; defm dollar_ok : BooleanFFlag<"dollar-ok">, Group<gfortran_Group>; ---------------- Is this option available in `gfortran`? If not we shouldn't be adding `Group<gfortran_Group>` here. ================ Comment at: flang/include/flang/Frontend/CompilerInvocation.h:22-28 +struct DialectOptions { + DialectOptions() {} + bool dfltReal{false}; + bool dfltInt{false}; + bool dfltDouble{false}; + bool largeSizes{false}; +}; ---------------- Do we really need this? if yes, `FrontendOptions.h` would probably be a better place for this. ================ Comment at: flang/include/flang/Frontend/CompilerInvocation.h:80-81 + // Fortran Dialect options + std::unique_ptr<Fortran::common::IntrinsicTypeDefaultKinds> defaultKinds_; + Fortran::frontend::DialectOptions dialectOpts_; + ---------------- Wouldn't it be possible to just use `defaultKinds_` here rather than `defaultKinds_` _and_ `dialectOpts_`? Do you think that we will need both? ================ Comment at: flang/lib/Frontend/CompilerInvocation.cpp:261 + diags.getCustomDiagID(clang::DiagnosticsEngine::Error, + "Use of `-fdefault-double-8` requires `-fdefault-real-8`"); + diags.Report(diagID); ---------------- Is this requirement document anywhere? ================ Comment at: flang/test/Flang-Driver/pipeline.f90:1-2 +! Test that flang can forward all of the flags which are documented as +! being supported by gfortran to flang-new for a fortran input file. +! ---------------- I suspect that this is C&P error from [[ https://github.com/llvm/llvm-project/blob/e6a62ac62571229d941dfe81affabdbc47e478eb/clang/test/Driver/gfortran.f90 | here ]] :) IIUC, this file tests that the compiler driver (`flang-new`) forwards all Flang frontend options to the compiler frontend driver (`flang-new -fc1`) as expected. Could you update the description? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96344/new/ https://reviews.llvm.org/D96344 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
