This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Inconsistent behavior regarding line break before access modifier
ClosedPublic

Authored by Max_S on Mar 29 2021, 6:28 AM.

Details

Summary

Fixes https://llvm.org/PR41870.

Checks for newlines in option Style.EmptyLineBeforeAccessModifier are now based on the formatted new lines and not on the new lines in the file.

Diff Detail

Event Timeline

Max_S requested review of this revision.Mar 29 2021, 6:28 AM
Max_S created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 29 2021, 6:28 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Max_S added a project: Restricted Project.Mar 29 2021, 6:32 AM

This is patch for a bug reported in 2019 https://bugs.llvm.org/show_bug.cgi?id=41870

lebedev.ri retitled this revision from Fixes for bug 41870. to [clang-format] Fixes for bug 41870..Mar 29 2021, 6:36 AM
MyDeveloperDay retitled this revision from [clang-format] Fixes for bug 41870. to [clang-format] Inconsistent behavior regarding line break before access modifier.
MyDeveloperDay edited the summary of this revision. (Show Details)

can you clang-format so it passes the pre-merge checks

MyDeveloperDay added inline comments.Apr 14 2021, 5:26 AM
clang/unittests/Format/FormatTest.cpp
8962

if you left a line between this test or added your test at the end it wouldn't have cut the existing test in two. I can't easily tell if you are changing or adding tests

Look good in general, only the few comments.

clang/unittests/Format/FormatTest.cpp
8892

While we are at empty lines, maybe add some here, so that it is a bit more readable. It took me some time to see that you use 2 different styles.

Max_S updated this revision to Diff 337665.Apr 15 2021, 1:52 AM

Moved the test to their own section and fixed the formatting issues.

Max_S marked 2 inline comments as done.Apr 15 2021, 1:54 AM
curdeius accepted this revision.Apr 15 2021, 3:27 AM

No remarks from me. LGTM.

This revision is now accepted and ready to land.Apr 15 2021, 3:27 AM
curdeius edited the summary of this revision. (Show Details)Apr 15 2021, 3:28 AM
Max_S updated this revision to Diff 337691.Apr 15 2021, 3:44 AM
Max_S edited the summary of this revision. (Show Details)

Added changelog entry.

Max_S added a comment.Apr 15 2021, 3:45 AM

I do not have the access rights to the llvm git repo. Can you please land it for me.

Thanks and thank you for the review.

I don't know if you did elsewhere, but you have to give a name and email for the commit, so that someone can push it for you.

Sorry forgot about that: Max Sagebaum <max.sagebaum@scicomp.uni-kl.de>

@Max_S I think you need to rebase on current main and fix two tests as probably D98237 (or another patch) changed the behaviour of this patch.

[  FAILED  ] 2 tests, listed below:
[  FAILED  ] FormatTest.FormatsAccessModifiers
[  FAILED  ] FormatTest.FormatsAfterAccessModifiers
Max_S updated this revision to Diff 338011.Apr 16 2021, 12:41 AM

Sorry for the inconvenience. These were the errors, that inspired this patch.

In all cases the new version is now the correct one. The default setting of EmptyLineBeforeAccessModifier is to insert empty lines for logical blocks, which it does now.