git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Stefan Beller <sbeller@google.com>
To: Jeff King <peff@peff.net>
Cc: git <git@vger.kernel.org>
Subject: Re: [PATCH] color.h: document and modernize header
Date: Thu, 8 Feb 2018 13:04:08 -0800
Message-ID: <CAGZ79kb+xEVZagqqNCHCPQUbfH89N7fdrO7dp6WHRGgJJje28Q@mail.gmail.com> (raw)
In-Reply-To: <20180208204309.GA4541@sigill.intra.peff.net>

On Thu, Feb 8, 2018 at 12:43 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Feb 08, 2018 at 12:15:46PM -0800, Stefan Beller wrote:
>
>>  int color_fprintf(FILE *fp, const char *color, const char *fmt, ...)
>>  {
>>       va_list args;
>> diff --git a/color.h b/color.h
>> index fd2b688dfb..8c7e6c41c2 100644
>> --- a/color.h
>> +++ b/color.h
>> @@ -72,26 +72,50 @@ extern int color_stdout_is_tty;
>>   * Use the first one if you need only color config; the second is a convenience
>>   * if you are just going to change to git_default_config, too.
>>   */
>> -int git_color_config(const char *var, const char *value, void *cb);
>> -int git_color_default_config(const char *var, const char *value, void *cb);
>> +extern int git_color_config(const char *var, const char *value, void *cb);
>> +extern int git_color_default_config(const char *var, const char *value, void *cb);
>
> Hmph, I thought we weren't adding "extern" everywhere. See:
>
>   https://public-inbox.org/git/xmqq8tea5hxi.fsf@gitster.mtv.corp.google.com/
>
> Other than that, these changes mostly look like improvements. A few

...

> Those are all suggestions. Given that there's no documentation currently
> on most of these, I think even if you don't take any of my suggestions,
> this would still be a net improvement (modulo the "extern" thing).

A funny and sad rant about why clear communication matters:

[Once upon a time, maybe 2 years ago] I had the impression that the old
code is nicely written and was consistently marked extern in header files.
(which btw is consistent with variable declarations, they need the extern).
All the new code doesn't make use of extern, so I had this on my low prio
todo list, that eventually all code converges to have 'extern'
functions in headers.

C.f. the following commits, found via
  git log -p --author=Beller -S extern

  5ec8274b84 (xdiff-interface: export comparing and hashing strings,
  2017-10-25) adding new externs

  1ecbf31d02 (hashmap: migrate documentation from Documentation/technical
  into header, 2017-06-30), a cleanup, which doesn't touch externs

  a6d7eb2c7a (pull: optionally rebase submodules (remote submodule
  changes only), 2017-06-23) new code using externs

  bd26756112 (submodule.h: add extern keyword to functions, 2016-12-20)
  (The commit message is as accurate as it gets)

You may sense a pattern here: I currently have the very firm understanding
we use the extern keyword in our codebase.

And I can also attest that this was not always the case, as back in the
day I remember writing patches without the extern keyword only to be told:
(A) be similar to the function in the next lines
(B) the standard is to use extern
and I was convinced it was a bad decision to prefix declarations with
the extern keyword, but followed along as I don't want to have style
in the way of writing features.

  $ cat Documentation/CodingGuidelines |grep extern
  $ # oh no it's empty!

Care to add a section to our coding guidelines?

Thanks,
Stefan

  reply index

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-02-08 20:15 Stefan Beller
2018-02-08 20:43 ` Jeff King
2018-02-08 21:04   ` Stefan Beller [this message]
2018-02-08 21:38     ` [PATCH] CodingGuidelines: mention "static" and "extern" Jeff King
2018-02-08 21:43       ` Stefan Beller
2018-02-08 23:14       ` Eric Sunshine
2018-02-09 19:07         ` Jonathan Tan
2018-02-09 19:33           ` Jeff King
2018-02-08 22:26   ` [PATCH] color.h: document and modernize header Eric Sunshine
2018-02-08 22:28     ` Jeff King
2018-02-12 20:19       ` Stefan Beller
2018-02-12 22:14         ` Eric Sunshine
2018-02-13  1:41           ` Stefan Beller
2018-02-13  3:55             ` Eric Sunshine
2018-02-14  7:23               ` Eric Sunshine
2018-02-14 17:58                 ` Stefan Beller

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAGZ79kb+xEVZagqqNCHCPQUbfH89N7fdrO7dp6WHRGgJJje28Q@mail.gmail.com \
    --to=sbeller@google.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox