[clang] [clang] Assert the enum FPOpts and LangOpts fit into the storage (PR #126166)

Florian Mayer via cfe-commits cfe-commits at lists.llvm.org
Mon Feb 10 14:30:54 PST 2025


fmayer wrote:

> > > > > Not sure I understand your question? Which assertion? `clang.exe t.c` is triggering an assertion?
> > > > 
> > > > 
> > > > I don't understand why you say
> > > > > The value `CX_None` is the defaut value given to the `Range` when no `fcomplex-arithmetic` is used on the command lin
> > > > 
> > > > 
> > > > but imply it does not need to be stored in the bitfield. If you patch this CL, and change the size for `ComplexRange` back, you will see it that it fails the assertion I added if you run the clang test suite.
> > > 
> > > 
> > > OK. I see the issue. Thanks. I think this should be titled [NFC][Clang] YOUR TITLE. If you don't want to make it NFC then you need to add a LIT test :-) Not sure you can make it fail with a LIT test.
> > 
> > 
> > What do you mean? Of course you can't fail the assertion now that the offending code is fixed.
> 
> I added your patch to my code base (added only the assert, didn't change the size of storage for `ComplexRange`) and was able to confirm that the assertions go off. So you are right. I am agreeing to merge this patch.
> 
> However the rule is that every patch needs at least one LIT test that illustrates the issue and prove that your patch fixes it. You don't have a LIT test in your patch. So, you can either make this patch `NFC` by adding it in your title or you need to produce a LIT test.

Just adding the `assert` makes a whooping 20594 out of 21977 tests fail. Does it really need to be 20595? If you want to test the functionality without the assert, be my guest, but I don't think that's a useful use of time.

This patch is not NFC because it fixes buggy behaviour. If you look closely, your None value actually overflowed into unrelated bits for the FpOpts, causing who knows what behavior to change.

https://github.com/llvm/llvm-project/pull/126166


More information about the cfe-commits mailing list