Chuck,
Thank you for the review - I know it is a lot of code to go through! I have
implemented most of your suggestions but I still object your recommendation to
define constants for -X return codes - see my comments below.
I'm preparing patch with these changes and some other fixes. I'll send it to you
when it is ready.
Best,
Rafal
Chuck Bell wrote:
>
> Rafal,
>
> This is a tough one to call. Since I could not apply the patches, I used
> your repository that you made available to me. Unfortunately, it does not
> compile on Windows generating 69 errors and 39 warnings.
>
> So I have to say this patch is not ok and we should work together to get the
> compile and patch problems sorted out before this patch is pushed. It also
> makes me concerned for the other 3 patches but fortunately they are much
> smaller.
>
> I will also try compiling the code on Linux tomorrow. I am not concerned so
> much for the warnings, but some of the errors I am seeing leads me to think
> there may be code or class-level design changes needed to correct them.
>
> Compiler Errors on Windows
> --------------------------
> C2593 - operator == is ambiguous
> C3861 - identifier not found (can be corrected with EXTRA_DEBUG declaration)
> C2065 - 'SQLCOM_BACKUP' identifier not found (and SQLCOM_RESTORE ...)
> C2051 - case expression not constant
>
> Compiler Warnings on Windows
> ----------------------------
> C4291 - no matching operator found
> C4355 - "this" used in base member initializer list
> C4099 - type name first seen using 'struct'
> C4065 - switch contains default but no case labels
>
Let's try to resolve these problems together - I need you expertise in windows
building environment. I'm trying to get access to our replication windows server
but I'm not sure if I manage and if it will be possible to compile there
(ideally share a desktop).
If this doesn't work we can do it over skype and or IRC: you will compile and
show me the errors and I'll look at the code and try to figure out what needs to
be changed.
> Concerns about the code in this patch
> -------------------------------------
> Comments need spell checking. "paralled" "lenght"
>
I run the spell checker again - only found the "lenght" misspelling.
> There are a couple of places that have commented out code. If you really
> want this code to stay where it is, please place a comment block around it
> and explain why it is there else please remove them from the code.
>
> I think it would be best for our 5.2 baseline to remove all of the "old"
> code from these patches. Please remove them from the 5.2 patches, but
> perhaps leave them in the prototype repository. I think the code would be
> clearer without the extra methods.
>
Good point. I removed all old code - it is stored in the prototype tree.
> I still don't like the use of ordinals for return codes to trigger modes or
> switches (return -7 means nothing to me and the use of -7 isn't documented
> well). I strongly recommend using enums or const instead. Besides making the
> code more readable, the names of the enums or consts would also help
> comprehension considerably.
>
I hope you read my explanations for this design in LLD of WL#3904 (section
"Return values").
To give an example - suppose you call mysql_backup() and it returns -3. What
does that mean? Primarily, it means "An error happened during execution and it
was reported via backup logger". That's it - any other non-zero value has the
same meaning.
This is the main purpose of the return value which has only these two meanings:
error or no-error. But, to add flexibility, there is an "unofficial" extension
of this idea. Namely, functions return different non-zero values from different
places where errors were detected. If for some reason a caller needs to know
where the error happened inside mysql_backup() it can consult function code and
see that -3 is returned after failed backup of table data, to distinguish it
from two other places where errors are detected. Now, if the same value -3 is
returned from e.g. write_table_data() it still means "an error has happened" but
now it indicates different place which is inside this function (it is error when
adding backup pump to the scheduler).
If we want to define constants for all these return codes we would have as many
of them as there are functions reporting errors times the number of places
inside one function where errors are detected. This is a huge number and a lot
of work to enter all these constants (I'd even have problem with inventing names
for them). Then there is extra work in maintaining the constants when code
changes. And what we get in return? I claim that almost nothing. I don't think
we add to the clarity of the code with these heaps of constants with not-so-well
defined meaning. Besides, as I explain in WL we *don't want* to precisely
describe error which has happened with the return value - the only information
we want to pass is error/non-error distinction. Anything beyond that (i.e.
different non-zero values) should be considered an "undocumented" extension
which can be safely ignored by people who don't know about it.
> Questions (for Chuck's curiosity)
> ---------------------------------
> What is the <pre> tag for?
>
@pre and @post (this is how they should be written in doxygen, I think) mean
"pre-condition" and "post-condition" for a function.
> What should backup::BUSY be used for and when? I think some places I use
> "PROCESSING" when I may need to use "BUSY."
>
The two replies have the same effect that get/send_data() call will be repeated.
The difference is in telling the kernel if the data in the buffer is used by
driver or not.
- BUSY reply means that driver can't process current get/send_data() call and
thus the data in the buffer is not used - kernel is free to reuse buffer (its
memory are) for other purposes if it likes,
- PROCESSING reply means that driver started to process the get/send_data()
request but is not finished yet. Kernel will call get/send_data() again to check
when the request is completed. In each of these calls *the same buffer* will be
passed as driver is possibly using data in the buffer (perhaps there is a
separate thread reading/writing it). Thus the buffer is "blocked" by the driver
and can't be reused.
> General Comments
> ----------------
> I've noticed you don't follow the same doxygen block format everywhere. Some
> places have:
>
> /**
> * My comment
> * My comment
> */
>
> While others have:
>
> /**
> My comment
> My comment
> */
>
> Which is correct? Please change all comment blocks so they use the same
> format. BTW: If the second one is the correct way, let me know so I can
> change my code too! ;)
>
Looking at other parts of the code it seems that the second variant is being
used (sorry...). I changed all comments in my code to that format.
> Some single line comments use // instead of /* */.
>
Fixed.
> Please remove extra *'s from block comments that use the format:
>
> /***********************
> My comment
> ***********************/
>
Not sure what you mean by "extra *'s". I've removed stars in front of comment
content lines.
Rafal
