Hello,
Roy Lyseng a écrit, Le 04.04.2011 16:23:
> Hi Guilhem,
>
> the fix is OK to push. I have one suggestion below, and generally I have
> a couple of dislikes, but they can be removed later if we agree on them:
>
> 1. The need for the local variable thd everywhere
Looks like this one can't be avoided without some major work. thd is an
execution context, and we need an execution context...
> 2. Accessing LEX data through item->unit->thd->lex.
I think you mentioned that LEX would go away one day.
> Thanks,
> Roy
> ...
>
> On 04.04.11 15.45, Guilhem Bichot wrote:
>> #At file:///home/mysql_src/bzrrepos_new/59852/ based on
> revid:tor.didriksen@stripped
>>
>> 3358 Guilhem Bichot 2011-04-04
>> Code cleanup: getting rid of two redundant THD pointers in subquery-related
> classes
>> @ sql/item_subselect.cc
>> Item_subselect::thd and subselect_engine::thd are deleted.
>> The first is replaced with the existing Item_subselect::unit::thd.
>> The second one is replaced with the existing
> subselect_engine::item::unit::thd
>> (same object as for the first one, actually: item points to the
> containing
>> Item_subselect); for subselect_union_engine it's additionally possible to
> use
>> subselect_union_engine::unit::thd.
>> -bool Item_subselect::fix_fields(THD *thd_param, Item **ref)
>> +bool Item_subselect::fix_fields(THD *thd, Item **ref)
>> {
>> - char const *save_where= thd_param->where;
>> + char const *save_where= thd->where;
>> uint8 uncacheable;
>> bool res;
>>
>> DBUG_ASSERT(fixed == 0);
>> - engine->set_thd((thd= thd_param));
>> + /*
>> + Pointers to THD must match. unit::thd may vary over the lifetime of the
>> + item (for example triggers, and thus their Item-s, are in a cache shared
>> + by all connections), but reinit_stmt_before_use() keeps it up-to-date,
>> + which we check here. subselect_union_engine functions also do sanity
>> + checks.
>> + */
>> + DBUG_ASSERT(thd == unit->thd);
>> + // Engine accesses THD via its 'item' pointer, check it:
>> + engine->assert_item(this);
>
> I dislike having a function just for asserting. What about
> DBUG_ASSERT(this == engine->get_item()); ?
I find that get_item() is more open to "non-encapsulated usage" than
assert_item(), because with get_item() I get the Item and can then do
anything on it, whereas with assert_item() I can only check that the
Item is the same as I have i.e. I gain access to nothing more.
> get_item() may even be defined within #if defined(DEBUG) - #endif
Ah, indeed, I can do that, and then get_item() doesn't give access to
anything, as it doesn't exist in release builds.
New patch committed. Will seek a second reviewer now.
