fix: added new BANNED_EXPL macro for better error messages, new parameter
PATCH 1/1 fix: added new BANNED_EXPL macro for better error messages, new parameter
From: HGimself <[email protected]>
---
fix: added new BANNED_EXPL macro for better error messages, has a par…
Extend Banned Function Error Messages
=====================================
AS A NEWER USER, I WANT TO BE ABLE TO UNDERSTAND WHY CERTAIN FUNCTIONS ARE BANNED AS WELL AS KNOW WHAT FUNCTIONS TO USE INSTEAD.
Changes
=======
* Added new macro named BANNED_EXPL(func, expl) which added the expl onto the error message
* Used strcpy as an example
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-896%2FHGHimself%2Ffix%2Fadd-new-ban-macro-with-explanation-parameter-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-896/HGHimself/fix/add-new-ban-macro-with-explanation-parameter-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/896
banned.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
banned.h +2 -1
@@ -9,9 +9,10 @@*/ #define BANNED (func) sorry_##func##_is_a_banned_function +#define BANNED_EXPL (func, expl) sorry_##func##_is_a_banned_funcion_because_##expl##.#undef strcpy -#define strcpy (x,y) BANNED(strcpy) +#define strcpy (x,y) BANNED_EXPL(strcpy, buffer_overflow_risk)#undef strcat#define strcat (x,y) BANNED(strcat)#undef strncpy
base-commit: be7935ed8bff19f481b033d0d242c5d5f239ed50
Signature · 1 line
gitgitgadget
banned.h line 15
Quoted 3 lines · HG King via GitGitGadget
#undef strcpy
banned.h +1
-1-#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.
Quoted 9 lines · Junio C Hamano
banned.h line 15
Quoted 3 lines · HG King via GitGitGadget
#undef strcpy
banned.h +1
-1-#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.
Quoted 4 lines
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
Quoted 9 lines · Taylor Blau
Quoted 4 lines
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
Quoted 9 lines · Taylor Blau
Quoted 4 lines
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.
To help those who are not yet familiar with this codebase but are willing to learn, I tend to agree with you that it is a good idea not to miss opportunities to encourage them to run "git blame" to find out about the project history in the parts of the system that pique their interest. They will make more motivated contributors who are invested in the project, if they stick around.
It would discourage and filter out "drive-by" contributors, though.
Somebody may not yet be interested enough to "git clone" us, but happens to have an extract of distro source tarball and enough motivation to try peeking and tweaking around. To such a curious developer, the "blame" information and log messages with the change are not readily available, so there probably is some balance to be struck.