[PATCH] D152473: [clang-format] Add test case for issue 63170

owenpan added a comment. In D152473#4410030 , @paulkirth wrote: > In D152473#4409975 , > @MyDeveloperDay wrote: > >> In D152473#4409146 , @paulkirth >> wrote: >> >>> The

[PATCH] D152473: [clang-format] Add test case for issue 63170

MyDeveloperDay added a comment. Part of my concern was that the reversion removed a unit test, which effectively regressed a different fix, I'm not comfortable with that. I think we can resolve the original problem with the following fix. diff --git a/clang/lib/Format/UnwrappedLineFormatter.c

[PATCH] D152473: [clang-format] Add test case for issue 63170

paulkirth marked an inline comment as done. paulkirth added a comment. In D152473#4410331 , @MyDeveloperDay wrote: > My additional concern is, is the original patch is the root cause of the > regression?, so I’m struggling to understand why this in part

[PATCH] D152473: [clang-format] Add test case for issue 63170

MyDeveloperDay added a comment. My additional concern is, is the original patch is the root cause of the regression?, so I’m struggling to understand why this in particular is being reverted or are we just going backwards through all commits? Repository: rG LLVM Github Monorepo CHANGES SINC

[PATCH] D152473: [clang-format] Add test case for issue 63170

phosek added a comment. I understand the concerns and I apologize if my LGTM came out as disrespectful, but there has been an issue reported with the original change over two days ago, including a reproducer, and given given that this issue is

[PATCH] D152473: [clang-format] Add test case for issue 63170

paulkirth added a comment. In D152473#4409975 , @MyDeveloperDay wrote: > In D152473#4409146 , @paulkirth > wrote: > >> The point of this patch was to show a regression. I'm not trying to fix >> anything per se.

[PATCH] D152473: [clang-format] Add test case for issue 63170

MyDeveloperDay added a comment. In D152473#4409652 , @phosek wrote: > LGTM @phosek can I ask you review https://llvm.org/docs/CodeReview.html When providing an unqualified LGTM (approval to commit), it is the responsibility of the reviewer to have re

[PATCH] D152473: [clang-format] Add test case for issue 63170

MyDeveloperDay added a comment. In D152473#4409146 , @paulkirth wrote: > The point of this patch was to show a regression. I'm not trying to fix > anything per se. I'd like https://reviews.llvm.org/D151954 and anything that > depends on it reverted, si

[PATCH] D152473: [clang-format] Add test case for issue 63170

MyDeveloperDay reopened this revision. MyDeveloperDay added a comment. This revision is now accepted and ready to land. Did this need an immediate revert? was the unit tests failing? Comment at: clang/test/Format/overlapping-lines.cpp:1 +// RUN: grep -Ev "// *[A-Z-]+:" %s | cla

[PATCH] D152473: [clang-format] Add test case for issue 63170

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGb6a718016c0f: [clang-format] Add test case for issue 63170 (authored by paulkirth). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D152473: [clang-format] Add test case for issue 63170

phosek accepted this revision. phosek added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152473/new/ https://reviews.llvm.org/D152473 ___

[PATCH] D152473: [clang-format] Add test case for issue 63170

paulkirth updated this revision to Diff 530004. paulkirth edited the summary of this revision. paulkirth added reverted changes: rGd2627cf88d25: [clang-format] Add the KeepEmptyLinesAtEOF option, rG4b9764959dc4: [clang-format] Fix overlapping replacements before PPDirectives, D152305: [clang-form

[PATCH] D152473: [clang-format] Add test case for issue 63170

paulkirth updated this revision to Diff 52. paulkirth added a comment. Revert patches introducing regression & add unit test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152473/new/ https://reviews.llvm.org/D152473 Files: clang/docs/ClangFo

[PATCH] D152473: [clang-format] Add test case for issue 63170

paulkirth added a comment. The point of this patch was to show a regression. I'm not trying to fix anything per se. I'd like https://reviews.llvm.org/D151954 and anything that depends on it reverted, since it breaks `clang-format`. My understanding was that @owenpan was reluctant to revert sin

[PATCH] D152473: [clang-format] Add test case for issue 63170

MyDeveloperDay added a comment. I presume the test fail, we don't check in tests that just fail without a fix, or the build would be stale. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152473/new/ https://reviews.llvm.org/D152473 ___

[PATCH] D152473: [clang-format] Add test case for issue 63170

paulkirth added a comment. In D152473#4407832 , @owenpan wrote: > Can you add the test to clang/unittests/Format/FormatTest.cpp instead? Please > also include the full diff. See > https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-in

[PATCH] D152473: [clang-format] Add test case for issue 63170

owenpan added a comment. Can you add the test to clang/unittests/Format/FormatTest.cpp instead? Please also include the full diff. See https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://re

[PATCH] D152473: [clang-format] Add test case for issue 63170

paulkirth created this revision. paulkirth added a reviewer: owenpan. Herald added projects: All, clang, clang-format. Herald added a subscriber: cfe-commits. Herald added reviewers: rymiel, HazardyKnusperkeks, MyDeveloperDay. paulkirth requested review of this revision. Herald added a comment. NO