[PATCH] D87587: [clang-format][PR47290] Add ShortNamespaceLines format option

[Krystian Kuzniarek via Phabricator via cfe-commits]

kuzkry marked 3 inline comments as done.
kuzkry added a comment.

In D87587#2552314 <https://reviews.llvm.org/D87587#2552314>, @curdeius wrote:

> Please regroup tests and add release notes.

Regroup tests - by that I think you meant all your 4 remarks about the tests. So I've done 3/4 :D
Release notes - done



================
Comment at: clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp:1126
+
+  EXPECT_EQ(UnwrappedLines, Style.ShortNamespaceLines);
+}
----------------
curdeius wrote:
> This could be tested together with another test.
I merged it with "OneUnwrappedLine_*" tests


================
Comment at: clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp:1129
+
+TEST_F(ShortNamespaceLinesTest,
+       ZeroUnwrappedLines_DoesNotAddEndCommentForOneLinerNamespace) {
----------------
curdeius wrote:
> In general, I think you can merge some test cases to match the current testing "style".
Sorry, I didn't understand what you meant so I skipped this one for now.


================
Comment at: clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp:1161
+TEST_F(ShortNamespaceLinesTest,
+       OneUnwrappedLine_AddsEndCommentForLongNamespace) {
+  EXPECT_EQ("namespace LongNamespace {\n"
----------------
curdeius wrote:
> You should check that Style.ShortNamespaceLines is 1 here.
My initial idea was to have a separated test that would validate ShortNamespaceLines defaulting to "1". If that test failed, someone would know why and they might think:
"Oh my, I think I broke something, let's see what. Oh, I see the test is called "DefaultsToOneUnwrappedLine" so I probably changed this default". Now, some example test, say "OneUnwrappedLine_AddsEndCommentForLongNamespace", does two things, which is a bit like trying to kill two birds with one stone making it, imho, a bit inferior.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87587/new/

https://reviews.llvm.org/D87587