Page MenuHomePhabricator

[clang][cli] NFC: Make parsing macro reusable
ClosedPublic

Authored by jansvoboda11 on Dec 22 2020, 6:50 AM.

Details

Summary

This is necessary for a future patch, where we start using this macro in another function.

Diff Detail

Event Timeline

jansvoboda11 requested review of this revision.Dec 22 2020, 6:50 AM
jansvoboda11 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 22 2020, 6:50 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Add newline at the end of file, simplify macro forwarding

dexonsmith added inline comments.Dec 22 2020, 11:18 PM
clang/lib/Frontend/CompilerInvocation.cpp
3050–3051

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.

jansvoboda11 added inline comments.Jan 4 2021, 5:36 AM
clang/lib/Frontend/CompilerInvocation.cpp
3050–3051

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.

dexonsmith added inline comments.Jan 5 2021, 11:50 AM
clang/lib/Frontend/CompilerInvocation.cpp
3050–3051

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).

3327–3328

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...)

Pass local variables to macros explicitly, rebase on top of prep patch.

jansvoboda11 added inline comments.Jan 6 2021, 8:36 AM
clang/lib/Frontend/CompilerInvocation.cpp
3050–3051

I've dropped the unused parameters for the PARSE_ macro. D84673 does the same for the GENERATE_ macro.

3327–3328

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.

This revision is now accepted and ready to land.Jan 6 2021, 12:36 PM
jansvoboda11 retitled this revision from [clang][cli] NFC: Make marshalling macros reusable to [clang][cli] NFC: Make parsing macro reusable.Jan 7 2021, 5:21 AM
jansvoboda11 edited the summary of this revision. (Show Details)
This revision was landed with ongoing or failed builds.Jan 7 2021, 5:26 AM
This revision was automatically updated to reflect the committed changes.