dexonsmith 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" ---------------- 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). ================ Comment at: clang/lib/Frontend/CompilerInvocation.cpp:3331-3332 + +#undef GENERATE_OPTION_WITH_MARSHALLING +#undef PARSE_OPTION_WITH_MARSHALLING ---------------- 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...) 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