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

[Evgeniy Stepanov]

Like this.


On Wed, Jan 16, 2013 at 10:39 AM, Evgeniy Stepanov
<eugeni.stepanov at gmail.com> wrote:
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 1.patch
Type: application/octet-stream
Size: 957 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130116/86f0b1e5/attachment.obj>