From: Date: October 2 2008 11:13am
Subject: Re: bzr commit into mysql-6.0-backup branch (oystein.grovlen:2699)
 Bug#39089 WL#4384
List-Archive: http://lists.mysql.com/commits/55028
Message-Id: <48E490B6.9000804@sun.com>
MIME-Version: 1.0
Content-Type: text/plain; format=flowed; charset=UTF-8
Content-Transfer-Encoding: 8BIT

Hi Oystein,

Øystein Grøvlen wrote:
> Rffal,
> 
> Thanks for the review.  Responses to requests and suggestiongs below.  I 
> will address the commentary part in a separate email.
> 

Ok. As you know, in the review process only requests are important. If you don't 
agree with some of my suggestions you can simply ignore them - I do not expect 
you to explain your decisions here. But, of course, it is always interesting to 
exchange opinions if you like it :)

>>
>> REQUESTS
>> --------
>>
>> 1. In Logger::write_message() (logger.cc), please fix the error which 
>> was present there. In case of reporting errors using 
>> push_warning_printf() the constant should be 
>> MYSQL_ERROR::WARN_LEVEL_ERROR, not MYSQL_ERROR::WARN_LEVEL_WARN.
> 
> I am not quite sure this is the right patch to fix this.  It is kind of 
> unrelated to what I try to achieve, and it seems it also involves fixing 
> at least one result file.  I think it is better to make this a separate 
> patch since it would then be easier to spot the specific change.
> 

Do you suggest to open a new bug for this, to witch the separate patch will belong?

>>
>> 4. Implementation of Backup_info::Global_iterator class. Either keep 
>> the current design of "construction is initialization", in which case 
>> there is no init() method, or switch to using *::init() method 
>> consistently for all iterators. The second alternative would mean 
>> adding init() to the public interface of Image_info::Iterator and 
>> implementing it in derived classes.
> 
> "Construction is initialization" is not achievable without using 
> exceptions since that is the only way to abort a construction upon errors.
>

I don't think it is not achievable. You can implement a class so that in case of 
errors during construction an invalid instance is created which can not be used 
(e.g. other methods error if called or this is guarded with assertions forcing 
the user of the class to check for instance validity before using it).

So, one can do this without exceptions. A separate question is whether such 
implementation makes sense. I'm not sure and don't want to argue for that. My 
only request is to be consistent across the different iterators.

> On the other hand, adding init() methods that will lead me into the same 
> trouble as we discussed before.  The iterators are created by functions 
> that return pointers to the created objects.  Since many of them does 
> not have the context to log errors, there is no way to report the errors 
> that init() may generate.  (Global_iterator is different since it is a 
> inner class to Backup_info).  Hence, information about the error will be
> lost.  From a pragmatic point of view, that is probably not an issue 
> since these init methods will currently not generate any errors anyway. 
>      So I will go ahead and add init() methods.

This is fine with me, as long as you mean adding init() to Image_info::Iterator 
and implementing it in all derived iterator classes.

>> SUGGESTIONS
>> -----------
>>
>> 1. Implement Logger::push_errors() as inlined method in logger.h.
> 
> This is not according to the code standard:
> 
> "Do not make a function inline if you don't have a very good reason for 
> it. In many cases, the extra code that is generated is more likely to 
> slow down the resulting code than give a speed increase because the 
> bigger code will cause more data fetches and instruction misses in the 
> processor cache."
> 
> I do not think there is any very good reasons for making error handling 
> code inline.  This is code that that is not part of the normal execution 
> part.
>

On the other hand, push_errors() is just one or two assignments so having an 
explicit function call for it could be an overkill. Unfortunately, the coding 
standard doesn't specify what is a "very good reason" and the evaluation is 
quite subjective. This is why I put it as suggestion, not a requirement. Please 
ignore it if you don't see a "very good reason" here.

Rafal