Repository navigation
[clang-format] Use 64-bit penalties in the optimizing line formatter - #236087
yashaggarwal85d wants to merge 1 commit into
Conversation
|
Hello @yashaggarwal85d 👋 Thank you for submitting a Pull Request (PR) to the LLVM Project. Since this is your first PR, here are a few useful links covering our main contribution policies and review practices.
Please reply to this message to confirm that you have read these policies, especially the LLVM AI Tool Use Policy, and that any AI tool usage has been noted in the PR description. Frequently asked questionsHow do I add reviewers? This PR will be automatically labeled, and the relevant teams will be notified. For some parts of the project, reviewers may also be added automatically. You can also add reviewers manually using the Reviewers section on this page. If you cannot use that section, it is probably because you do not have write permissions for the repository. In that case, you can request a review by tagging reviewers in a comment using What if there are no comments? If you have not received any comments on your PR after a week, you can request a review by pinging the PR with a comment such as “Ping”. The common courtesy ping rate is once a week. Please remember that you are asking for volunteer time from other developers. Are any special GitHub settings required to contribute to LLVM? We only require contributors to have a public email address associated with their GitHub commits, see this section of LLVM Developer Policy for details. If you have questions, feel free to leave a comment on this PR, or ask on LLVM Discord or LLVM Discourse. Thank you, |
|
@llvm/pr-subscribers-clang-format Author: Yash Aggarwal (yashaggarwal85d) ChangesThe penalty in OptimizingLineFormatter::analyzeSolutionSpace() was summed in an unsigned. On a long braced list where every row goes over the column limit, the total goes past 2^32 and wraps, so a worse layout can end up with the smaller penalty. With 81 rows of 100-digit literals the brace moved to its own line and everything got indented 4 more. This keeps the search penalty (and OrderedPenalty) in 64 bits, and clamps the value returned from formatLine() to UINT_MAX so callers that add it up don't wrap either. Penalties of a single token can still overflow inside ContinuationIndenter if one token is thousands of columns past the limit. I left that alone to keep this small. Fixes #235788 Full diff: https://github.com/llvm/llvm-project/pull/236087.diff 2 Files Affected:
diff --git a/clang/lib/Format/UnwrappedLineFormatter.cpp b/clang/lib/Format/UnwrappedLineFormatter.cpp
index d1f68877ef0339..bb44d134f3edf4 100644
--- a/clang/lib/Format/UnwrappedLineFormatter.cpp
+++ b/clang/lib/Format/UnwrappedLineFormatter.cpp
@@ -1425,7 +1425,7 @@ class OptimizingLineFormatter : public LineFormatter {
/// In case of equal penalties, we want to prefer states that were inserted
/// first. During state generation we make sure that we insert states first
/// that break the line as late as possible.
- typedef std::pair<unsigned, unsigned> OrderedPenalty;
+ typedef std::pair<uint64_t, unsigned> OrderedPenalty;
/// An edge in the solution space from \c Previous->State to \c State,
/// inserting a newline dependent on the \c NewLine.
@@ -1468,7 +1468,7 @@ class OptimizingLineFormatter : public LineFormatter {
Queue.push(QueueItem(OrderedPenalty(0, Count), RootNode));
++Count;
- unsigned Penalty = 0;
+ uint64_t Penalty = 0;
// While not empty, take first element and follow edges.
while (!Queue.empty()) {
@@ -1517,14 +1517,14 @@ class OptimizingLineFormatter : public LineFormatter {
<< "Total number of analyzed states: " << Count << "\n");
LLVM_DEBUG(llvm::dbgs() << "---\n");
- return Penalty;
+ return std::min<uint64_t>(Penalty, UINT_MAX);
}
/// Add the following state to the analysis queue \c Queue.
///
/// Assume the current state is \p PreviousNode and has been reached with a
/// penalty of \p Penalty. Insert a line break if \p NewLine is \c true.
- void addNextStateToQueue(unsigned Penalty, StateNode *PreviousNode,
+ void addNextStateToQueue(uint64_t Penalty, StateNode *PreviousNode,
bool NewLine, unsigned *Count, QueueType *Queue) {
if (NewLine && !Indenter->canBreak(PreviousNode->State))
return;
@@ -1533,9 +1533,11 @@ class OptimizingLineFormatter : public LineFormatter {
StateNode *Node = new (Allocator.Allocate())
StateNode(PreviousNode->State, NewLine, PreviousNode);
- if (!formatChildren(Node->State, NewLine, /*DryRun=*/true, Penalty))
+ unsigned ChildrenPenalty = 0;
+ if (!formatChildren(Node->State, NewLine, /*DryRun=*/true, ChildrenPenalty))
return;
+ Penalty += ChildrenPenalty;
Penalty += Indenter->addTokenToState(Node->State, NewLine, true);
Queue->push(QueueItem(OrderedPenalty(Penalty, *Count), Node));
diff --git a/clang/unittests/Format/FormatTest.cpp b/clang/unittests/Format/FormatTest.cpp
index f38287757ffba1..df971a4c513845 100644
--- a/clang/unittests/Format/FormatTest.cpp
+++ b/clang/unittests/Format/FormatTest.cpp
@@ -20440,6 +20440,15 @@ TEST_F(FormatTest, BreakPenaltyScopeResolution) {
Style);
}
+TEST_F(FormatTest, PenaltyDoesNotOverflow) {
+ // The total penalty of this line is above 2^32.
+ std::string Code = "int t[] = {\n";
+ for (int I = 0; I < 81; ++I)
+ Code += " 0x" + std::string(100, '0') + ",\n";
+ Code += "};";
+ verifyFormat(Code);
+}
+
TEST_F(FormatTest, WorksFor8bitEncodings) {
// FIXME: unstable test case
EXPECT_EQ("\"\xce\xe4\xed\xe0\xe6\xe4\xfb \xe2 \"\n"
|
|
FYI, AI was used to write clang/unittests/Format/FormatTest.cpp file changes. |
The penalty in OptimizingLineFormatter::analyzeSolutionSpace() was summed in an unsigned. On a long braced list where every row goes over the column limit, the total goes past 2^32 and wraps, so a worse layout can end up with the smaller penalty. With 81 rows of 100-digit literals the brace moved to its own line and everything got indented 4 more.
This keeps the search penalty (and OrderedPenalty) in 64 bits, and clamps the value returned from formatLine() to UINT_MAX so callers that add it up don't wrap either.
Penalties of a single token can still overflow inside ContinuationIndenter if one token is thousands of columns past the limit. I left that alone to keep this small.
Fixes #235788