JakeMerdichAMD edited reviewers, added: MyDeveloperDay; removed: jmerdich. JakeMerdichAMD requested changes to this revision. JakeMerdichAMD added a subscriber: MyDeveloperDay. JakeMerdichAMD added a comment. This revision now requires changes to proceed.
After looking at more of the history (ie, the commit you referenced), I'd definitely be open to something like this, provided that it doesn't affect namespaces that reside completely on one line. Since it was mostly a clang-format limitation and relatively rare, I think we can change the default here, but that's not up to just me (+@MyDeveloperDay), and extra scrutiny is definitely required when changing existing tests. In any case, can you also add the full diff context <https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface>? It makes it easier for us to review. ================ Comment at: clang/unittests/Format/NamespaceEndCommentsFixerTest.cpp:426-428 + EXPECT_EQ("namespace A { a }// namespace A", + fixNamespaceEndComments("namespace A { a }")); + EXPECT_EQ("namespace A { a };// namespace A", ---------------- I strongly believe that these ones are not correct. Namespaces that are entirely on one line should never have a trailing comment, even if it has content in it. A solution would also have to take into account whether future passes would split this onto separate lines (and thus have a different result after re-running clang-format), which was the reason for this limitation in the first place. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87587/new/ https://reviews.llvm.org/D87587 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
