He Zhenxing wrote:
>> REQUESTS
>> --------
>>
>> 1. You extend mysqlbinlog.test but I don't see the corresponding changes in the
>> result file (forgot to --record new results?). This should be fixed.
>>
>
> The test code I added does not generate any output in the result file, I
> did this on purpose, the 'diff_files' line checks weather this part of
> the test passed or not.
>
Right, now I noticed the disable_query_log directive. In that case patch is ok
to push.
>> SUGGESTIONS
>> -----------
>>
>> 2. Declare **defaults_argv as static variable to discourage its use from other
>> modules.
>> 3. I don't like that main() is using defaults_argv variable which is not
>> assigned within it. This obscures the logic and makes code harder to
>> understand/maintain. I suggest to go around this by declaring a new function
>> with the following specification:
>>
>> /**
>> @function free_args()
>>
>> @short Frees memory allocated by parse_args(). Should be always called after
>> a call to parse_args().
>> */
>>
>> Then this function would be called from within main(). Of course you still need
>> a global variable to communicate between free_args() and parse_args(), but the
>> use of that variable would be limited to these two functions and this will be
>> easier to understand.
>>
>
> I am thinking about another way now, moving the load_defaults out of
> parse_args() to main(), so that we do not need to make defaults_argv
> global.
>
Looks good.
Rafal
