Chuck,
This is the patch with post-review fixes <http://lists.mysql.com/commits/29697>.
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
>
> Concerns about the code in this patch
> -------------------------------------
> Comments need spell checking. "paralled" "lenght"
>
> 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.
>
> 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.
>
> Questions (for Chuck's curiosity)
> ---------------------------------
> What is the <pre> tag for?
>
> What should backup::BUSY be used for and when? I think some places I use
> "PROCESSING" when I may need to use "BUSY."
>
> 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! ;)
>
> Some single line comments use // instead of /* */.
>
> Please remove extra *'s from block comments that use the format:
>
> /***********************
> My comment
> ***********************/
>
>
> Chuck
