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

[Evgeniy Stepanov]

r172614.

On Wed, Jan 16, 2013 at 11:34 AM, Evgeniy Stepanov
<eugeni.stepanov at gmail.com> wrote:
> 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