This is an archive of the discontinued LLVM Phabricator instance.

fixes bug #51926 where dangling comma caused overrun
ClosedPublic

Authored by feg208 on Sep 25 2021, 9:14 AM.

Details

Summary

bug 51926 identified an issue where a dangling comma caused the cell count to be to off by one

Diff Detail

Event Timeline

feg208 requested review of this revision.Sep 25 2021, 9:14 AM
feg208 created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2021, 9:14 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
clang/unittests/Format/FormatTest.cpp
17805

Here is no alignment, maybe add another case with another line?

What happens if one line has the trailing comma and the next doesn't?

feg208 added inline comments.Sep 25 2021, 3:34 PM
clang/unittests/Format/FormatTest.cpp
17805

yesh this is pretty ugly. In the second case it crashes. This isn't going to be fixed for a bit

clang/unittests/Format/FormatTest.cpp
17805

What is that bit? Now we can still revert the original change for LLVM 13 (at least I think so, since there are new RC Tags on GitHub), or can fix the crash.

feg208 added inline comments.Sep 26 2021, 5:24 PM
clang/unittests/Format/FormatTest.cpp
17805

I think we should fix the crash since it’s only an issue with the dangling comma but if the consensus is we should revert then I can do that as well

HazardyKnusperkeks added inline comments.
clang/unittests/Format/FormatTest.cpp
17805

Totally agree, could you please add the test cases? The ones that crash you could comment out and fix afterwards.

I don't know how much time we have before the next version is released.

feg208 added inline comments.Sep 27 2021, 4:37 PM
clang/unittests/Format/FormatTest.cpp
17805

Let me see if I can fix the weirdness with the second test case but I'll time box it to two days so this doesn't drag out. I'd feel better about it not crashing just because we added an additional row.

feg208 updated this revision to Diff 375544.Sep 28 2021, 5:52 AM

Gets rid of the crash for the second added case

So I added in the second case but commented it out as it gets misformatted. However it doesn't assert which was the alarming outcome. I have one other bug to clean up here and I am going to open a bug to fix the line breaking after initial { in certain cases since the code there needs to be revisited. I am doing formatting in the info gathering loop where really I should add all that in the back end. But thats a bigger change that I'd like to do outside of mitigating this users crash.

By one other bug I mean an entirely separate bug that I will handle in a separate PR. Sorry if that was confusing.

By one other bug I mean an entirely separate bug that I will handle in a separate PR. Sorry if that was confusing.

Okay, but this should be handled quickly.

This revision is now accepted and ready to land.Sep 28 2021, 1:27 PM

Absolutely in agreement

This revision was landed with ongoing or failed builds.Sep 28 2021, 3:59 PM
This revision was automatically updated to reflect the committed changes.