Re: bzr commit into mysql-5.0 branch (joro:2823) Bug#42760

[Alexey Kopytov]

On 06.11.09 15:36, Georgi Kodinov wrote:
> Hi,
>
> Thanks for looking at the code.
>
> On 06.11.2009, at 14:25, Alexey Kopytov wrote:
>
>>> +INSERT INTO t1 VALUES (1, 10), (2, NULL);
>>> +
>>> +--echo # Must use ref-or-null on the a_c index
>>
>> Why?
>
> To be able to run through the fixed code of course :-)
> I generally like putting comments in the test cases explaining what
> they're *really* testing. Makes it a lot more easier to read the and
> maintain test case.
>

I think adding EXPLAINs to regression tests as a way to ensure coverage 
for a particular code path is a bad idea for quite a number of reasons.

Starting this discussion as a part of a code review does not make sense, 
of course (I already tried). For now, I'm just collecting reasons why 
people do it.

>>> === modified file 'sql/sql_select.cc'
>> [skipped]
>>> --- a/sql/sql_select.cc 2009-10-21 09:04:08 +0000
>>> +++ b/sql/sql_select.cc 2009-10-22 16:39:31 +0000
>>> @@ -6336,6 +6342,7 @@ make_join_readinfo(JOIN *join, ulonglong
>>> break;
>>> default:
>>> DBUG_PRINT("error",("Table type %d found",tab->type)); /* purecov:
>>> deadcode */
>>> + DBUG_ASSERT (1 != 0);
>>
>> Why?
>
>
> The explanation is on the previous line : dead code. I needed to make
> sure it stays the same. In the same time if I remove it gcc will
> generate warnings that I want to avoid. Hmm, should it be DBUG_ASSERT (1
> == 0) to always trigger an assertion ?
>

It should be DBUG_ASSERT(0). Just got confused by the constantly true 
assertion, thanks for explanations.

Best regards,
Alexey.