On 2020/02/29 0:46, Hamid Akhtar wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world: not tested
> Implements feature: not tested
> Spec compliant: not tested
> Documentation: not tested
>
> First of all, this seems like fixing a valid issue, albeit, the probability of somebody messing is low, but it is
stillbetter to fix this problem.
>
> I've not tested the patch in any detail, however, there are a couple of comments I have before I proceed on with
detailedtesting.
Thanks for the review and comments!
> 1. pgindent is showing a few issues with formatting. Please have a look and resolve those.
Yes.
> 2. I think you can potentially use "len" variable instead of introducing "buflen" and "tmplen" variables.
Basically I don't want to use the same variable for several purposes
because which would decrease the code readability.
> Also, I would choose a more appropriate name for "tmp" variable.
Yeah, so what about "rest" as the variable name?
> I believe if you move the following lines before the conditional statement and simply and change the if statement to
"if(len >= sizeof(buf) - 1)", it will serve the purpose.
ISTM that this doesn't work correctly when the "buf" contains
trailing carriage returns but not newlines (i.e., this line is too long
so the "buf" doesn't include newline). In this case, pg_strip_crlf()
shorten the "buf" and then its return value "len" should become
less than sizeof(buf). So the following condition always becomes
false unexpectedly in that case even though there is still rest of
the line to eat.
> + if (len >= sizeof(buf) - 1)
> + {
> + char tmp[LINELEN];
Regards,
--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters