This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by HazardyKnusperkeks 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.Apr 16 2021, 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.Apr 16 2021, 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.Apr 21 2021, 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.Apr 21 2021, 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.Apr 21 2021, 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.May 7 2021, 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.May 8 2021, 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.May 8 2021, 2:53 AM
clang/unittests/Format/FormatTest.cpp
3526

I want to be sure I understand you the right way before pushing changes.
Now the clang-format for your code gives the following formatting:

switch (x) {
  class S1 {};
  class S2 : S1 {};
case 0:
  class S3 {};
  class S4 : public S3 {};
};

Is it okay?

Apart from that comment I don't have any objections. :)

clang/include/clang/Format/Format.h
2025–2033

You don't write the .. code-block your self.

@curdeius could you please write your opinion?

curdeius added inline comments.May 19 2021, 4:10 AM
clang/unittests/Format/FormatTest.cpp
3526

Hey, your test looks good. Sorry for the delay.

@MyDeveloperDay @HazardyKnusperkeks could you please clarify is there something I need to change in patch or we can push it to the next stage?

For me it's good, but please wait for @MyDeveloperDay .

MyDeveloperDay added a comment.EditedMay 21 2021, 9:17 AM

Did you prove that CLASS(x) : won't be an issue?

or

return  x > y ? FOO(x) : BAR(y)
MyDeveloperDay requested changes to this revision.May 21 2021, 9:31 AM
class FOO(x) : public x {
  public:
    int x;

    FOO(int y) : x(y) {}
};

will become

class FOO(x) : public x {
  public:
    int x;

FOO(int y) :
    x(y) {}
};

with this patch.

This revision now requires changes to proceed.May 21 2021, 9:31 AM

Did you prove that CLASS(x) : won't be an issue?

or

return  x > y ? FOO(x) : BAR(y)

I will add test for it soon. I checked these cases locally and no issue found.

@MyDeveloperDay could you please clarify why unit tests are failed on Phabricator and there is no diff in logs (what is expected), but locally I tested the same tests and the result was successful?

@MyDeveloperDay could you please clarify why unit tests are failed on Phabricator and there is no diff in logs (what is expected), but locally I tested the same tests and the result was successful?

These tests fail for me with your current revision.

@MyDeveloperDay How can I see the formatted result to compare it with expected (to know what exactly wrong)? Now I see only these in logs on Phabricator:

Script:
--
/var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/unittests/Format/./FormatTests --gtest_filter=FormatTest.StatementMacroInSwitchBlock
--
Note: Google Test filter = FormatTest.StatementMacroInSwitchBlock
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from FormatTest
[ RUN      ] FormatTest.StatementMacroInSwitchBlock
/var/lib/buildkite-agent/builds/llvm-project/clang/unittests/Format/FormatTest.cpp:45: Failure
Expected equality of these values:
  ExpectedCompleteFormat
    Which is: true
  Status.FormatComplete
    Which is: false
class FOO(x) : public x {
public:
  int x;
  FOO(int y) : x(y) {}
};


Google Test trace:
/var/lib/buildkite-agent/builds/llvm-project/clang/unittests/Format/FormatTest.cpp:3864: class FOO(x) : public x { public: int x; FOO(int y) : x(y) {} };
/var/lib/buildkite-agent/builds/llvm-project/clang/unittests/Format/FormatTest.cpp:45: Failure
Expected equality of these values:
  ExpectedCompleteFormat
    Which is: true
  Status.FormatComplete
    Which is: false
class FOO(x) : public x { public: int x; FOO(int y) : x(y) {} };

@MyDeveloperDay after my research I found that tests fail because the status of lines below formatting are marked as FormatComplete = false.

FOO(int y) : x(y) {}\n"
"return x > y ? FOO(x) : BAR(y);"

It means:

/// A value of ``false`` means that any of the affected ranges were not
  /// formatted due to a non-recoverable syntax error.

Does it mean that the test itself is marked as invalid?

I'm not sure of the reason for the failure without debugging it, We cannot commit with failing tests so we have to determine the reason why.

MyDeveloperDay added inline comments.May 26 2021, 1:30 AM
clang/lib/Format/UnwrappedLineParser.cpp
2920

Can you comment by what you mean by "InlevelParsing"?

I'm not sure of the reason for the failure without debugging it, We cannot commit with failing tests so we have to determine the reason why.

I checked this test without my patch and I have the same result. I think I need to understand why this test cause a syntax error firstly.

if you think this is done you need to mark the comments as done if you have addressed them

MyDeveloperDay requested changes to this revision.Nov 4 2021, 2:42 AM
This revision now requires changes to proceed.Nov 4 2021, 2:42 AM
HazardyKnusperkeks commandeered this revision.Nov 8 2022, 12:53 PM
HazardyKnusperkeks edited reviewers, added: anastasiia_lukianenko; removed: HazardyKnusperkeks.
Herald added a project: Restricted Project. · View Herald TranscriptNov 8 2022, 12:53 PM

More than one year silence.