[llvm-commits] [llvm] r172535 - /llvm/trunk/include/llvm/IR/Instructions.h

[David Blaikie]

> No and no. One function of asserts is documenting the code, and this one was
> misleading and wrong.

Sure - I didn't mean to imply it was otherwise. But a wrong assert
could be one of two things:

1) too lax - doesn't fire in cases where it should
2) too strict - fires in cases where it shouldn't

Adding a test for (1) shouldn't be possible. The assert should never
fire in valid executions of the program (so any test would represent a
bug).

(2) should be testable, though - if the assert is too strict, it
should be firing for some valid programs. We should add a test with
such a program.

> Or did i misunderstood something? This difference between constructors was
> added in Nadav's change about a year ago, I assumed it's a mistake.

Again, I don't disagree - I assume it was a mistake (I haven't looked
at the code or changes in detail, I'm happy to take your word for it).
But every mistake that isn't failing test cases tells us there's a
missing test case, ideally.

- David

>
> On Jan 15, 2013 9:47 PM, "David Blaikie" <dblaikie at gmail.com> wrote:
>>
>> On Tue, Jan 15, 2013 at 7:30 AM, Evgeniy Stepanov
>> <eugeni.stepanov at gmail.com> wrote:
>> > Author: eugenis
>> > Date: Tue Jan 15 09:30:33 2013
>> > New Revision: 172535
>> >
>> > URL: http://llvm.org/viewvc/llvm-project?rev=172535&view=rev
>> > Log:
>> > Fix operand type conditions in one of ICmpInst constructors.
>> >
>> > It was out of sync with the conditions in the other two constructors.
>>
>> Is there a case where this assert used to erroneously fire? Do you
>> have a test case for that?
>>
>> >
>> > Modified:
>> >     llvm/trunk/include/llvm/IR/Instructions.h
>> >
>> > Modified: llvm/trunk/include/llvm/IR/Instructions.h
>> > URL:
>> > http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Instructions.h?rev=172535&r1=172534&r2=172535&view=diff
>> >
>> > ==============================================================================
>> > --- llvm/trunk/include/llvm/IR/Instructions.h (original)
>> > +++ llvm/trunk/include/llvm/IR/Instructions.h Tue Jan 15 09:30:33 2013
>> > @@ -953,7 +953,7 @@
>> >            "Both operands to ICmp instruction are not of the same
>> > type!");
>> >      // Check that the operands are the right type
>> >      assert((getOperand(0)->getType()->isIntOrIntVectorTy() ||
>> > -            getOperand(0)->getType()->isPointerTy()) &&
>> > +            getOperand(0)->getType()->getScalarType()->isPointerTy())
>> > &&
>> >             "Invalid operand types for ICmp instruction");
>> >    }
>> >
>> >
>> >
>> > _______________________________________________
>> > llvm-commits mailing list
>> > llvm-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits