[llvm] r184239 - We want a string representation of the attribute, not the kind as a string.

[David Blaikie]

On Tue, Jun 18, 2013 at 5:28 PM, Bill Wendling <isanbard at gmail.com> wrote:
> On Jun 18, 2013, at 5:10 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>> On Tue, Jun 18, 2013 at 4:46 PM, Bill Wendling <isanbard at gmail.com> wrote:
>>> On Jun 18, 2013, at 3:43 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>
>>>> On Tue, Jun 18, 2013 at 2:52 PM, Bill Wendling <isanbard at gmail.com> wrote:
>>>>> On Jun 18, 2013, at 2:39 PM, Bill Wendling <isanbard at gmail.com> wrote:
>>>>>
>>>>>> On Jun 18, 2013, at 2:35 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>>>>
>>>>>>> On Tue, Jun 18, 2013 at 2:27 PM, Bill Wendling <isanbard at gmail.com> wrote:
>>>>>>>> Author: void
>>>>>>>> Date: Tue Jun 18 16:27:00 2013
>>>>>>>> New Revision: 184239
>>>>>>>>
>>>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=184239&view=rev
>>>>>>>> Log:
>>>>>>>> We want a string representation of the attribute, not the kind as a string.
>>>>>>>
>>>>>>> Test case? (do we have test cases for the verifier?)
>>>>>>>
>>>>>> I'll cobble one together.
>>>>>>
>>>>> I don't think I can. This requires that the .bc file be malformed. So the `llvm-as' will reject it before it gets to the code I just fixed...
>>>>
>>>> Can we remove this as dead code then? (replace it with an assert)
>>>
>>> No. The check is valid for checking bitcode rather than text.
>>
>> Be nice to have tests for it, then - even if they're checked in binary
>> files. (I'd be OK with them being derived from known good files with a
>> comment (in the corresponding 'foo.test' text file that actually has
>> the RUN line in it) explaining which bit was twiddled)
>>
>> Do we not have any tests for binary-only issues the verifier can diagnose?
>>
> No. And the file they sent is rather large...and because of the nature of the bug, I can't reduce it.

Not sure I follow as to why it would be hard to produce an artificial
case that would reach the code based on an understanding of the code
(since you know what the bug is) rather than based on a manual
reduction from the reported sample (when you don't know where the bug
is I can understand the need to reduce the original test case in an
effort to help isolate the piece of code to fix the bug in)

> I *really* think it's okay that this go in without a test. :-)

I *really* dislike untested code (let alone substantial chunks of code
without even a test strategy - in part because every person who goes
to fix a bug here will have the same incremental reason not to bother
adding a test case because there aren't any others/no obvious way to
do it... so we end up with a growing body of untested code in an area
that it's sort of important that we have good coverage because I
expect users who need the verifier would really like it to work
correctly).

- David