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 still better 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 detailed testing.
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
