"HG King via GitGitGadget" [off-list ref] writes:
quoted hunk
#undef strcpy
-#define strcpy(x,y) BANNED(strcpy)
+#define strcpy(x,y) BANNED_EXPL(strcpy, buffer_overflow_risk)
That does not help programmers that much (the above does not say
what to use instead, for example), and the mechanism inherently
does not give you sufficient space to give helpful guidance.
Adding a comment around each of these definition may be OK. Upon
seeing foo_is_a_banned_function, somebody new to the codebase would
look for where it is banned, and find the above, so that is a good
place to give guidance.
On Sun, Mar 07, 2021 at 12:34:57PM -0800, Junio C Hamano wrote:
"HG King via GitGitGadget" [off-list ref] writes:
quoted
#undef strcpy
-#define strcpy(x,y) BANNED(strcpy)
+#define strcpy(x,y) BANNED_EXPL(strcpy, buffer_overflow_risk)
That does not help programmers that much (the above does not say
what to use instead, for example), and the mechanism inherently
does not give you sufficient space to give helpful guidance.
Trying to cram information like "why is this function unsafe?" and "what
function should I use instead?" seems ill-fitted to trying to a macro
which is supposed to have a field for each.
I'm certainly not opposed to making these banned functions clearer, but
I do not think that this is the way to do it.
Adding a comment around each of these definition may be OK. Upon
seeing foo_is_a_banned_function, somebody new to the codebase would
look for where it is banned, and find the above, so that is a good
place to give guidance.
Perhaps, but all of this information is already covered accurately in
the patches that introduced each banned function. So I'm not sure that I
even agree that this information is difficult to discover to begin with,
but I may be biased.
Thanks,
Taylor
On Mon, Mar 08, 2021 at 01:05:46PM -0500, Taylor Blau wrote:
quoted
Adding a comment around each of these definition may be OK. Upon
seeing foo_is_a_banned_function, somebody new to the codebase would
look for where it is banned, and find the above, so that is a good
place to give guidance.
Perhaps, but all of this information is already covered accurately in
the patches that introduced each banned function. So I'm not sure that I
even agree that this information is difficult to discover to begin with,
but I may be biased.
Yeah, certainly my intent when I introduced banned.h was that people
would get the full reasoning from the commit log. I figured Git
developers could run "git log -S" or "git blame".
I'm not opposed to comments if somebody wants to write them, but it's
not clear to whether people who are actively writing patches for Git
have actually run into this situation and been confused, or if this is
bikeshedding from the recent posting of banned.h on Hacker News. (Even
if it is the latter, I am OK taking a patch that adds comments; I just
doubt that it's a good use of anybody's time).
-Peff