jansvoboda11 added inline comments.
================ Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3040 -#define OPTION_WITH_MARSHALLING( \ - PREFIX_TYPE, NAME, ID, KIND, GROUP, ALIAS, ALIASARGS, FLAGS, PARAM, \ - HELPTEXT, METAVAR, VALUES, SPELLING, ALWAYS_EMIT, KEYPATH, DEFAULT_VALUE, \ - IMPLIED_CHECK, IMPLIED_VALUE, NORMALIZER, DENORMALIZER, MERGER, EXTRACTOR, \ - TABLE_INDEX) \ - if ((FLAGS)&options::CC1Option) { \ - this->KEYPATH = MERGER(this->KEYPATH, DEFAULT_VALUE); \ - if (IMPLIED_CHECK) \ - this->KEYPATH = MERGER(this->KEYPATH, IMPLIED_VALUE); \ - if (auto MaybeValue = \ - NORMALIZER(OPT_##ID, TABLE_INDEX, Args, Diags, Success)) \ - this->KEYPATH = MERGER( \ - this->KEYPATH, static_cast<decltype(this->KEYPATH)>(*MaybeValue)); \ - } - +#define OPTION_WITH_MARSHALLING PARSE_OPTION_WITH_MARSHALLING #include "clang/Driver/Options.inc" ---------------- dexonsmith wrote: > jansvoboda11 wrote: > > dexonsmith wrote: > > > One concern I have with this change is that the macro is using local > > > variable names; it really would be better to have the macro content local > > > to the function(s) where the variables are defined. > > > > > > Can you give more context about why this has to be called from another > > > place? Maybe there's another way to solve the problem. > > I've provided more context here: D93701. > > > > We can solve the problem with implicit use of local variables inside the > > macro by promoting them to macro parameters. > > This will make the forwarding call from `OPTION_WITH_MARSHALLING` to > > `PARSE_OPTION_WITH_MARSHALLING` longer, as it will need to spell out all > > parameters, but it would solve your concern I think. > I like that idea. That approach also allows you to drop unused parameters > entirely in `PARSE_OPTIONS_WITH_MARSHALLING` (only forward the parameters > that get used). I've dropped the unused parameters for the `PARSE_` macro. D84673 does the same for the `GENERATE_` macro. ================ Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3331-3332 + +#undef GENERATE_OPTION_WITH_MARSHALLING +#undef PARSE_OPTION_WITH_MARSHALLING ---------------- dexonsmith wrote: > I like the idea of `#undef`'ing these macros to make their scope of use > clear, but I'm not sure doing it at the end of file is adding a lot of value. > > Is there a way of moving the call sites for each to be next to each other in > the file, so you can `#define` and then `#undef` in a more limited scope? > E.g., > ``` > // maybe other content... > > #define PARSE_OPTION_WITH_MARSHALLING(...) ... > void f1(...) { /* use PARSE_OPTION_WITH_MARSHALLING */ } > void f2(...) { /* use PARSE_OPTION_WITH_MARSHALLING */ } > #undef PARSE_OPTION_WITH_MARSHALLING > > // maybe other content... > > #define GENERATE_OPTION_WITH_MARSHALLING(...) ... > void f3(...) { /* use GENERATE_OPTION_WITH_MARSHALLING*/ } > void f4(...) { /* use GENERATE_OPTION_WITH_MARSHALLING*/ } > #undef GENERATE_OPTION_WITH_MARSHALLING > > // maybe other content... > ``` > (If so, the code movement should be done in a separate NFC prep commit...) Yeah, limiting the scope where the macro is defined would be nice. I've moved things around and we'll be able to keep the `GENERATE_` macro local to the function, while the `PARSE_` macro will span only the two functions that'll make use of it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93702/new/ https://reviews.llvm.org/D93702 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits