| Message ID | 1584898493-27180-1-git-send-email-sheikhhamza012@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [GSoC] cache.h: change READ_GITFILE_ERR constants group to enum | expand |
sheikh hamza <sheikhhamza012@gmail.com> writes: > From: sheikh hamza <sheikhhamza012@gmail.com> Please make sure that the "your name <address>" on your From: header matches what you write on the Signed-off-by: trailer. > this patch is my microproject for the GSoC i converted > the group of constants in cache.h on line 573 to an enum > named read_gitfile_err to follow newer coding convension > i intend to contribute as much to git as i can and this is > my initial contribution i hope to get guidance for future > contribution. i would be working on the GSoC proposal any > help would be appreciated, Thank you Almost nothing in the above paragraph explains what this patch is about and why we should consider updating our code with it. It would be a good background story that can be told below the three-dash line, though. The body of the log message is where you explain what the change is about and justify why the change is a good idea. Both preprocessor macros (that is what these things are called; "constants" could mean "const int READ_GIFILE_ERR_STAT_FAILED = 1;" so avoid using the word to refer to them) and enums can be used to make the code more descriptive, so the code that processes the error code, e.g. int err; const char *path = read_gitfile_gently(git, &err); if (err == 7) die("not a repository: '%s'", git); can become more readable by using READ_GITFILE_ERR_NOT_A_REPO instead of a magic constant "7". But they help readability the same way, so that is not a justification to change preprocessor macro to enum. One reason why folks prefer enum over preprocessor macro is because some debuggers can show enum values symbolically, while macros are not even seen by the debuggers. For example: enum read_gitfile_err err; const char *path = read_gitfile_gently(git, &err); printf("the first letter is '%c'\n", path[0]); would segfault while attempting to call the printf(), when read_gitfile_gently() finds an error and returns NULL. In such a case, a debugger that knows about the type of variable 'err' can show you READ_GITFILE_ERR_NOT_A_REPO instead of 7 when you say "print err". I suspect however that this benefit is only possible when the type of err is known to be a particular enum. Note that I updated the type of 'err' in the second example, but you could have left the variable as "int" and it would have been perfectly valid C, but I do not think a debugger can infer that what is in err is one of the values taken from "enum read_gitfile_err". So, I suspect that this patch alone does not give us the potential "it helps the debugger" benefit, either. The callsites of this function that currently pass a pointer to a variable of type "int" need to be updated if we want to use it as the justification. So does the type of the latter parameter of read_gitfile_gently() function need to change, I think. > Signed-off-by: Muhammad Hamza <sheikhhamza012@gmail.com> > --- > cache.h | 19 +++++++++++-------- > 1 file changed, 11 insertions(+), 8 deletions(-) > > diff --git a/cache.h b/cache.h > index 37c899b..6aae035 100644 > --- a/cache.h > +++ b/cache.h > @@ -570,14 +570,17 @@ static inline enum object_type object_type(unsigned int mode) > */ > int is_nonbare_repository_dir(struct strbuf *path); > > -#define READ_GITFILE_ERR_STAT_FAILED 1 > -#define READ_GITFILE_ERR_NOT_A_FILE 2 > -#define READ_GITFILE_ERR_OPEN_FAILED 3 > -#define READ_GITFILE_ERR_READ_FAILED 4 > -#define READ_GITFILE_ERR_INVALID_FORMAT 5 > -#define READ_GITFILE_ERR_NO_PATH 6 > -#define READ_GITFILE_ERR_NOT_A_REPO 7 > -#define READ_GITFILE_ERR_TOO_LARGE 8 > +enum read_gitfile_err { > + READ_GITFILE_ERR_STAT_FAILED = 1, > + READ_GITFILE_ERR_NOT_A_FILE = 2, > + READ_GITFILE_ERR_OPEN_FAILED = 3, > + READ_GITFILE_ERR_READ_FAILED = 4, > + READ_GITFILE_ERR_INVALID_FORMAT = 5, > + READ_GITFILE_ERR_NO_PATH = 6, > + READ_GITFILE_ERR_NOT_A_REPO = 7, > + READ_GITFILE_ERR_TOO_LARGE = 8, > +}; > + > void read_gitfile_error_die(int error_code, const char *path, const char *dir); > const char *read_gitfile_gently(const char *path, int *return_error_code); > #define read_gitfile(path) read_gitfile_gently((path), NULL)
diff --git a/cache.h b/cache.h index 37c899b..6aae035 100644 --- a/cache.h +++ b/cache.h @@ -570,14 +570,17 @@ static inline enum object_type object_type(unsigned int mode) */ int is_nonbare_repository_dir(struct strbuf *path); -#define READ_GITFILE_ERR_STAT_FAILED 1 -#define READ_GITFILE_ERR_NOT_A_FILE 2 -#define READ_GITFILE_ERR_OPEN_FAILED 3 -#define READ_GITFILE_ERR_READ_FAILED 4 -#define READ_GITFILE_ERR_INVALID_FORMAT 5 -#define READ_GITFILE_ERR_NO_PATH 6 -#define READ_GITFILE_ERR_NOT_A_REPO 7 -#define READ_GITFILE_ERR_TOO_LARGE 8 +enum read_gitfile_err { + READ_GITFILE_ERR_STAT_FAILED = 1, + READ_GITFILE_ERR_NOT_A_FILE = 2, + READ_GITFILE_ERR_OPEN_FAILED = 3, + READ_GITFILE_ERR_READ_FAILED = 4, + READ_GITFILE_ERR_INVALID_FORMAT = 5, + READ_GITFILE_ERR_NO_PATH = 6, + READ_GITFILE_ERR_NOT_A_REPO = 7, + READ_GITFILE_ERR_TOO_LARGE = 8, +}; + void read_gitfile_error_die(int error_code, const char *path, const char *dir); const char *read_gitfile_gently(const char *path, int *return_error_code); #define read_gitfile(path) read_gitfile_gently((path), NULL)
this patch is my microproject for the GSoC i converted the group of constants in cache.h on line 573 to an enum named read_gitfile_err to follow newer coding convension i intend to contribute as much to git as i can and this is my initial contribution i hope to get guidance for future contribution. i would be working on the GSoC proposal any help would be appreciated, Thank you Signed-off-by: Muhammad Hamza <sheikhhamza012@gmail.com> --- cache.h | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)