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

Evgeniy Stepanov eugeni.stepanov at gmail.com
Tue Jan 15 22:39:07 PST 2013


Forging an input program that exercises this code may be complicated.
I could add a unit test for this, for example, by extending
InstructionsTest.VectorGep.

On Tue, Jan 15, 2013 at 11:29 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> 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



More information about the llvm-commits mailing list