List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:April 4 2011 7:28pm
Subject:Re: bzr commit into mysql-trunk branch (guilhem.bichot:3358)
View as plain text  
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.
Thread
bzr commit into mysql-trunk branch (guilhem.bichot:3358) Guilhem Bichot4 Apr
  • Re: bzr commit into mysql-trunk branch (guilhem.bichot:3358)Roy Lyseng4 Apr
    • Re: bzr commit into mysql-trunk branch (guilhem.bichot:3358)Guilhem Bichot4 Apr