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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
