On 12/10/2020 5:35 PM, Jakub Jelinek wrote:
On Thu, Dec 10, 2020 at 10:27:46AM -0600, Patrick McGehearty via Gcc-patches
wrote:
Thank you for your rapid feedback.
I'll fix the various formatting issues (spaces in the wrong places
and such as well as revise the Changelog magic) in the next submission.
It will wait for Joseph's review to also make any changes he suggests.
I'll also try to train myself to be more sensitive to gcc formatting
conventions while proofreading.
I'm reluctant to change or use XALLOCAVEC instead of alloca as that
is not the current style elsewhere in the routine.
If so, I can fix it incrementally. But, at least fix up the formatting,
that was the reason I've mentioned XALLOCAVEC, because the alloca call
formatting was off.
If there is a stated policy somewhere that says "replace alloca with
XALLOCA*
whenever a file is to be updated/changed, I'll follow that, but so far
I have not found that statement.
Without the statement, iIf it's a good idea to change alloca to XALLOCAVEC,
I'd support that as a separate patch. Currently there are 25 uses of alloca
in gcc/c-family/* and 6 uses of XALLOCA*.
There are more other uses of alloca in the gcc src tree if you are looking
to make a clean sweep of alloca. I was not reading this list when XALLOCA*
first started getting used (quick googling suggests it was a decade ago)
so I'm not
up on why it makes a difference.
No need to confuse the issues by doing some changes with the complex divide
patch and some changes separately. As we don't know when complex divide
will make it through review, if you get XALLOCAVEC changes committed
to c-cppbuiltin.c before complex divide is approved, I will have no
problem with
replacing alloca with XALLOCAVEC in my patch.
And I will certainly do my best to fix all formatting issues in either case.
On the strcpy, strncpy, and memcpy question, given short length of
the string being copied, I don't think it makes much difference.
The two other copy operations in the file are memcpy.
memcpy might be slightly better since it is generally more frequently
seen and more likely that gcc has special case code to inline
short fixed length memcpy as a few assignments. Even if both strncpy
and memcpy are inlined, the memcpy code may be simplier as it does
not need to be concerned with special treatment of nulls.
I'll change the strncpy to memcpy.
Even if short, strncpy is a badly designed API that in 99% cases just
shouldn't be used. Either the string is shorter than the passed argument,
then in most cases completely useless zeroes the rest of the buffer
(exception is security sensitive code that needs to overwrite everything
that has been before), or it is the same length and then one should use
strcpy instead, or there is string truncation which doesn't zero terminate,
which is very rarely something one wants to do.
I generally avoid strcpy for various reasons that may not apply in this
particular case.
I used strncpy simply because "I need to copy a string, what does that?
oh, strncpy."
I was not sensitive to the points you make about strncpy.
memcpy is clearly right for this use case.
Jakub
- patrick
