[PATCH] D110481: fixes bug #51926 where dangling comma caused overrun

Fred Grim via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Sep 27 16:37:28 PDT 2021


feg208 added inline comments.


================
Comment at: clang/unittests/Format/FormatTest.cpp:17805
+               "    Section{\n"
+               "            0, bar(),\n"
+               "            }\n"
----------------
HazardyKnusperkeks wrote:
> feg208 wrote:
> > HazardyKnusperkeks wrote:
> > > feg208 wrote:
> > > > HazardyKnusperkeks wrote:
> > > > > 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?
> > > > yesh this is pretty ugly. In the second case it crashes. This isn't going to be fixed for a bit
> > > 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.
> > 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
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110481/new/

https://reviews.llvm.org/D110481



More information about the cfe-commits mailing list