| Message ID | xmqq8scgzqis.fsf@gitster.c.googlers.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | clean up extern decl of functions | expand |
Hi Junio, On Thu, Oct 08, 2020 at 08:27:55AM -0700, Junio C Hamano wrote: > Among external function declarations, somehow only these two > functions that return pointer-to-function were declared with > "extern" in front. > > Ideally, we should standardise to _have_ explicit "extern" in front > for all function (and data) decls, but let's make things uniform > first. Bulk re-addition of extern can be done without any extra > difficulty with or without this change. Why are we re-introducing an explicit "extern"? Since function decls are extern by default, what do we gain by doing this? You mentioned in the past[0] I think there is a push to drop the "extern " from decls of functions in *.h header files. so are we reversing that push now? > Signed-off-by: Junio C Hamano <gitster@pobox.com> The code part looks good to me. Good catch. Thanks, Denton [0]: https://public-inbox.org/git/xmqqef67zz7u.fsf@gitster-ct.c.googlers.com/
Denton Liu <liu.denton@gmail.com> writes: > Hi Junio, > > On Thu, Oct 08, 2020 at 08:27:55AM -0700, Junio C Hamano wrote: >> Among external function declarations, somehow only these two >> functions that return pointer-to-function were declared with >> "extern" in front. >> >> Ideally, we should standardise to _have_ explicit "extern" in front >> for all function (and data) decls, but let's make things uniform >> first. Bulk re-addition of extern can be done without any extra >> difficulty with or without this change. > > Why are we re-introducing an explicit "extern"? Since function decls are > extern by default, what do we gain by doing this? > > You mentioned in the past[0] > > I think there is a push to drop the "extern " from decls of > functions in *.h header files. > > so are we reversing that push now? That is certainly on the table. Re-read what you quoted and realize that I was not expressing my opinion on the "push"; it was just stating that other reviewers seem to be in favor. See my other response why I think the "push" was a bad idea.
Junio C Hamano <gitster@pobox.com> writes: >> Why are we re-introducing an explicit "extern"? Since function decls are >> extern by default, what do we gain by doing this? >> >> You mentioned in the past[0] >> >> I think there is a push to drop the "extern " from decls of >> functions in *.h header files. >> >> so are we reversing that push now? > > That is certainly on the table. Re-read what you quoted and realize > that I was not expressing my opinion on the "push"; it was just > stating that other reviewers seem to be in favor. > > See my other response why I think the "push" was a bad idea. I'd elaborate a bit more. A proposed update to CodingGuidelines will be sent separately as a follow-up to this message. "Are we reversing that push now?" That is not a question I can unilaterally answer yes/no---I do not run dictatorship where I cannot survive without telling all the contributors how to cross their t's and dot their i's. There are things in our coding guidelines that tells me to do something differently from how I would, but I can adjust and survive if the primary benefit of having guidelines, i.e. making things uniform one way or the other, is net win. When a guideline turns out to be a bad idea, however, I can propose to change it. So can you or anybody else ;-) In that message, I just told Emily that there is a push to omit extern, in the sense that it was the opinion of the prevailing louder voices. Back then, I didn't have an opinion strong enough to favor either way myself, and I was willing to go with the majority if many contributors wanted to drop "extern" in the hope that it will result in quality code. But with the Makefile patch you posted with Dscho's review this morning, it has become apparent to me that it wasn't a great idea after all. It caused you to spend your time to write the RFC patch and come up with the regexp, and caused the project to spend reviewer bandwidth on the patch. Of course, how you spend your time is entirely up to you. But if you are going to contribute your time on this project, the project appreciates if the time is spent on things that make the codebase better. And to me, unlike to me "in the past[0]", it is reasonably clear that the push of omitting "extern" ended up wasting resources without doing much good for the project. Seeing that the pattern that were trying to be careful didn't catch the decls fixed by the patch you were responding to in this thread did not help improve my impression on the idea of omitting "extern". I think these two decls I touched in this patch were left behind when somebody, possibly you in b199d714 (*.[ch]: remove extern from function declarations using sed, 2019-04-29), tried to "clean up" the last time, because the pattern used in the conversion did not catch it.
diff --git a/git-compat-util.h b/git-compat-util.h index 7a0fb7a045..56e0e1e79d 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -491,9 +491,9 @@ static inline int const_error(void) void set_die_routine(NORETURN_PTR void (*routine)(const char *err, va_list params)); void set_error_routine(void (*routine)(const char *err, va_list params)); -extern void (*get_error_routine(void))(const char *err, va_list params); +void (*get_error_routine(void))(const char *err, va_list params); void set_warn_routine(void (*routine)(const char *warn, va_list params)); -extern void (*get_warn_routine(void))(const char *warn, va_list params); +void (*get_warn_routine(void))(const char *warn, va_list params); void set_die_is_recursing_routine(int (*routine)(void)); int starts_with(const char *str, const char *prefix);
Among external function declarations, somehow only these two functions that return pointer-to-function were declared with "extern" in front. Ideally, we should standardise to _have_ explicit "extern" in front for all function (and data) decls, but let's make things uniform first. Bulk re-addition of extern can be done without any extra difficulty with or without this change. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- * A patch before morning coffee, could be totally off the mark with trivial thinko. git-compat-util.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)