awarzynski added a comment. @FarisRehman , thank you for updating this! I have two high-level comments:
1. In your summary: > Change the way the driver handles end-of-line characters in macro definitions. That's a a bit misleading - which driver do you mean? You're not changing the behavior of the current driver, `flang`. Also, this functionality doesn't yet exist in the new driver, `flang-new`, so you're not changing it there either. You can probably remove this sentence and just make it clear that you meant the new driver. 2. Could you doxygen style comments for input and output parameters in the new methods that you introduce? Ta! https://www.doxygen.nl/manual/commands.html#cmdparam More comments inline. ================ Comment at: clang/lib/Driver/ToolChains/Flang.cpp:36 + const InputInfo &Input = Inputs[0]; + ---------------- This is not used until line 73. Perhaps better to define it near to where it's used? https://isocpp.org/wiki/faq/coding-standards#declare-near-first-use ================ Comment at: clang/lib/Driver/ToolChains/Flang.h:27-31 + /// Add the specified preprocessing options from Args to CmdArgs, + /// claiming the arguments used. + /// + /// \param Args The list of input arguments + /// \param CmdArgs The list of output arguments ---------------- 1. Could you clarify the difference between `Args` and `CmdArgs`? 2. Could you use doxygen's syntax to specify the parameter _direction_? E.g.: ``` /// \param [in] Args The list of input arguments /// \param [out] CmdArgs The list of output arguments ``` See https://www.doxygen.nl/manual/commands.html#cmdparam. ================ Comment at: flang/include/flang/Frontend/PreprocessorOptions.h:26 +public: + std::vector<std::pair<std::string, /*isUndef*/ bool>> Macros; + ---------------- `Macros` --> `macros_` :) ================ Comment at: flang/lib/Frontend/CompilerInvocation.cpp:213-214 +/// +/// \param ppOpts The preprocessor options (input) +/// \param opts The fortran options (output) +static void collectMacroDefinitions( ---------------- [nit] Ideally this would use doxygen style: https://www.doxygen.nl/manual/commands.html#cmdparam. For example: ``` /// \param [in] ppOpts The preprocessor options /// \param [out] opts The fortran options ``` Apologies for not making it clearer earlier. ================ Comment at: flang/lib/Frontend/CompilerInvocation.cpp:234 + if (macroName.size() == macro.size()) + macroBody = "1"; + else { ---------------- I think that it would make sense to add a note in the commit message that `-Dname` predefines `name` as a macro with definition 1. Basically, a note that all macros default to 1. ================ Comment at: flang/test/Flang-Driver/macro_def_undef.f90:19 + + +!-------------------------------------------- ---------------- [nit] Empty line not needed ================ Comment at: flang/test/Flang-Driver/macro_def_undef.f90:37 + +! Macro.F: +#ifdef X ---------------- [nit] Doesn't agree with the name of file. Also, I would just skip it. ================ Comment at: flang/test/Flang-Driver/macro_multiline.f90:15 + + +!------------------------------- ---------------- [nit] Empty line not needed ================ Comment at: flang/test/Flang-Driver/macro_multiline.f90:19 +!------------------------------- +! printf -- "-DX=A\\\\\nTHIS_SHOULD_NOT_EXIST_IN_THE_OUTPUT\n" | xargs flang-new -E %s +! CHECK:program a ---------------- [nit] Repetition of the `RUN` line ================ Comment at: flang/test/Flang-Driver/macro_multiline.f90:24 + +! Macro.F: +program X ---------------- [nit] Doesn't agree with the name of file. Also, I would just skip it. ================ Comment at: flang/test/Flang-Driver/macro_multiline.f90:25-26 +! Macro.F: +program X +end ---------------- I think that it would be useful to make this a bit stronger: ``` ! CHECK: {{^start a end$}} ! CHECK-NOT: THIS_SHOULD_NOT_EXIST_IN_THE_OUTPUT ! CHECK-NOT: this_should_not_exist_in_the_output START X END ``` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93401/new/ https://reviews.llvm.org/D93401 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
