Page MenuHomePhabricator

[clang-format] Fix CompactNamespaces corner case when AllowShortLambdasOnASingleLine/BraceWrapping.BeforeLambdaBody are set
AcceptedPublic

Authored by aybassiouny on Mar 20 2021, 9:52 PM.

Details

Summary

Summary CompactNamespaces config stops work when AllowShortLambdasOnASingleLine is set to false and BraceWrapping.BeforeLambdaBody is true

Input:

namespace out {
namespace in {
} 
} // namespace out::in

Expected output:

namespace out { namespace in {
}} // namespace out::in

Output from current binaries:

namespace out {
namespace in {
} 
} // namespace out::in

Config triggering the issue:

---
AllowShortLambdasOnASingleLine: None
BraceWrapping:
  BeforeLambdaBody :    true
BreakBeforeBraces: Custom
CompactNamespaces: true
...

What's happening: Seems there's a corner case when AllowShortLambdasOnASingleLine is false, and BraceWrapping.BeforeLambdaBody is true, that causes CompactNamespaces to stop working. The root cause seems to be isAllmanLambdaBrace, that assumes the namespace brace is a lambda and blocks compacting the namespace. The regression was probably introduced with this commit.

I have only added a UT covering the corner cases, please let me know if more coverage is needed, I am new to the codebase. One corner case I am aware of, is anonymous namespaces, which would not be affected by my Tok.Previous->Previous->is(tok::kw_namespace) check, but I am not sure what's the expected behavior there anyway.

Diff Detail

Event Timeline

aybassiouny requested review of this revision.Mar 20 2021, 9:52 PM
aybassiouny created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 20 2021, 9:52 PM
Wawha accepted this revision.Mar 21 2021, 9:08 AM

Thank you for the fix!

This revision is now accepted and ready to land.Mar 21 2021, 9:08 AM

Looks good. How about verifyFormat? Would that fail without your patch?

How about verifyFormat? Would that fail without your patch?

@HazardyKnusperkeks sorry, but not sure that I understand your point. Are you suggesting to use verifyFormat in my UT instead of EXPECT_EQ?

How about verifyFormat? Would that fail without your patch?

@HazardyKnusperkeks sorry, but not sure that I understand your point. Are you suggesting to use verifyFormat in my UT instead of EXPECT_EQ?

I'm pretty sure @MyDeveloperDay would ask for that. I would maybe use both. But what it really interesting for me if verifyFormat would fail, i.e. would clang-format misformat something previously correctly formatted.

How about verifyFormat? Would that fail without your patch?

@HazardyKnusperkeks sorry, but not sure that I understand your point. Are you suggesting to use verifyFormat in my UT instead of EXPECT_EQ?

I'm pretty sure @MyDeveloperDay would ask for that. I would maybe use both. But what it really interesting for me if verifyFormat would fail, i.e. would clang-format misformat something previously correctly formatted.

I would, see D98237: [clang-format] Option for empty lines after an access modifier. as to why.

aybassiouny updated this revision to Diff 338403.EditedSun, Apr 18, 5:46 PM
aybassiouny edited the summary of this revision. (Show Details)

Sorry for the delay!

After rechecking, turns out AllowShortLambdasOnASingleLine and BeforeLambdaBody both need to be turned on in order for the regression to be expressed, this affects the UT.

Also added verifyFormat check as suggested, it does not fail btw without this patch.

After rechecking, turns out AllowShortLambdasOnASingleLine and BeforeLambdaBody both need to be turned on in order for the regression to be expressed, this affects the UT.

Please update the summary with this information.

clang/unittests/Format/FormatTest.cpp
2594

Nit.

curdeius added inline comments.Mon, Apr 19, 12:45 AM
clang/lib/Format/TokenAnnotator.cpp
3498

Please add at least asserts for Tok.Previous and Tok.Previous->Previous.

aybassiouny edited the summary of this revision. (Show Details)Mon, Apr 19, 10:05 AM