Page MenuHomePhabricator

[clang-format] Handle statement macro expressions in switch block
Needs RevisionPublic

Authored by anastasiia_lukianenko on Nov 26 2020, 3:33 AM.

Details

Summary

Cases in switch block can be defined as macros. Now clang-format binpacks
them in one line. This patch make statement macro cases behavior as usual
'case' in switch block.
For example:

#define PROGRESS(x)              \
    ...                          \
    case PROG_##x

Formatting now:
switch (x)
{
case 0:
    ...
    PROGRESS(var):
    ...
}

With patch:
switch (x)
{
case 0:
    ...
PROGRESS(var):
    ...
}

Diff Detail

Event Timeline

anastasiia_lukianenko requested review of this revision.Nov 26 2020, 3:33 AM
anastasiia_lukianenko created this revision.
MyDeveloperDay added inline comments.
clang/lib/Format/UnwrappedLineParser.cpp
1054

Aren't you saying here every statement macro is a case statement? I don't think that is going to work everywhere?

MyDeveloperDay edited reviewers, added: MyDeveloperDay; removed: Restricted Project.Nov 26 2020, 5:01 AM

#clang-format is a project not a groups of reviewers

To determine who the reviewers are, its best to look at the "git log" in clang-format areas and see who is making the changes, or look at the OWNERSHIP files

I'm happy to review clang-format items when I can.

MyDeveloperDay requested changes to this revision.Nov 30 2020, 3:02 AM

I'm not completely convinced this is going to work, but I'm happy to be persuaded incase I misunderstood

This revision now requires changes to proceed.Nov 30 2020, 3:02 AM

This looks okay, I think. But it should definitely add a release note and extend the documentation on statement macros.
Now I'm thinking maybe it should add a new flag to activate that behavior?

This looks okay, I think. But it should definitely add a release note and extend the documentation on statement macros.
Now I'm thinking maybe it should add a new flag to activate that behavior?

The patch was originally made as a fix for unexpected behavior. I'm not sure that someone would use a variant where the macro is not recognized as a case block... That's why I don't think the new flag is really needed.

HazardyKnusperkeks requested changes to this revision.Fri, Apr 16, 11:45 AM

This looks okay, I think. But it should definitely add a release note and extend the documentation on statement macros.
Now I'm thinking maybe it should add a new flag to activate that behavior?

The patch was originally made as a fix for unexpected behavior. I'm not sure that someone would use a variant where the macro is not recognized as a case block... That's why I don't think the new flag is really needed.

I said maybe, I don't have a strong opinion on that one, for the documentation on the other hand I have one.

clang/unittests/Format/FormatTest.cpp
3502

Is this needed?

This revision now requires changes to proceed.Fri, Apr 16, 11:45 AM
clang/unittests/Format/FormatTest.cpp
3502

Yes, because this type of colon is recognized as CtorInitializerColon. Without SpaceBeforeCtorInitializerColon = false the result will be:

"FOO(var) :"
MyDeveloperDay added inline comments.Wed, Apr 21, 3:30 AM
clang/lib/Format/UnwrappedLineParser.cpp
2925

I just feel this constraint is too lose its say all macrostatements which have a trailing colon are a label?

MACRO(x) :

its hard to think of a non "trite" example that counters that but

CLASS(x) : public Foo
{
};

might be one such situation if user have added CLASS as a statement macro (which may/maynot be the correct thing to do)

to me I feel this needs something like the following to help disambiguate it? (thoughts?)

parseStatementMacro(bool inSwitchStatement){
  ...
if (inSwitchStatement && FormatTok->is(tok::colon))
    parseLabel();
 ...
}
clang/unittests/Format/FormatTest.cpp
3503

is there a reason you are not using verifyFormat()

HazardyKnusperkeks requested changes to this revision.Wed, Apr 21, 11:27 AM
HazardyKnusperkeks added inline comments.
clang/docs/ClangFormatStyleOptions.rst
3552 ↗(On Diff #338960)

This has to be done in Format.h and then updated by the DumpFormatStyles.py script.

clang/lib/Format/UnwrappedLineParser.cpp
2925

I was actually thinking if there is a case where : isn't a label, but yeah that seems to be such a case. That should at least be covered by the tests.

clang/unittests/Format/FormatTest.cpp
3502

Is there a test to cover plain LLVM style? If not please add one.

This revision now requires changes to proceed.Wed, Apr 21, 11:27 AM
clang/unittests/Format/FormatTest.cpp
3503

No, I can change if it's better to use verifyFormat()

clang/unittests/Format/FormatTest.cpp
3503

If input and result are the same it should almost always be verifyFormat. It's easier to read and it ensures that the formatting is stable, because it removes all whitespace.

clang/docs/ClangFormatStyleOptions.rst
3552 ↗(On Diff #338960)

It now is in Format.h, but the script did not run? :)

curdeius added inline comments.Fri, May 7, 2:33 AM
clang/lib/Format/UnwrappedLineParser.cpp
1044

Nit: please use CamelCase for parameters and local variables.

1209
2920
clang/unittests/Format/FormatTest.cpp
3526

Please add a test with a class inheritance inside switch.
Something including:

switch (x) {
class S1 {};
class S2 : S1 {};
case ...
    class S3 {};
    class S4 : public S3 {};
};
MyDeveloperDay requested changes to this revision.Sat, May 8, 2:53 AM
MyDeveloperDay added inline comments.
clang/include/clang/Format/Format.h
2033

if you change Format.h you have to run docs/tools/dump_format.py this will regenerate the doc/ClangFormatStyleOptions.rst file which need to be part of any review that touches Format.h

This revision now requires changes to proceed.Sat, May 8, 2:53 AM