All of lore.kernel.org
 help / color / mirror / Atom feed
From: David Aguilar <davvid@gmail.com>
To: Perry Hutchison <perryh@pluto.rain.com>
Cc: gitster@pobox.com, git@vger.kernel.org
Subject: Re: [PATCH] help: ensure that common-cmds.h is only included by help.c
Date: Sun, 14 Sep 2014 01:44:30 -0700	[thread overview]
Message-ID: <20140914084429.GA74582@gmail.com> (raw)
In-Reply-To: <541549fd.u4o+i0ruC2hh0cGO%perryh@pluto.rain.com>

On Sun, Sep 14, 2014 at 12:55:41AM -0700, Perry Hutchison wrote:
> Junio C Hamano <gitster@pobox.com> wrote:
> > On Sat, Sep 13, 2014 at 7:00 PM, Perry Hutchison <perryh@pluto.rain.com> wrote:
> > > David Aguilar <davvid@gmail.com> wrote:
> > >> Add a #ifndef guard to ensure that common-cmds.h can only
> > >> be included by help.c.
> > >
> > > ... If these definitions are intended to be private to help.c,
> > > why not put them there and eliminate common-cmds.h entirely?
> >
> > Have you studied where common-cmds.h comes from?
> 
> Not when I wrote that :)
> 
> > After you have done so, would you make the same suggestion?
> 
> Yes, as a matter of using C in a conventional and non-hackish
> manner.  The normal and expected purpose of .h files is to share
> definitions among compilation units.  If certain definitions are
> -- by design -- intended to be used in only a single compilation
> unit, they should not be put in a .h file; that only encourages
> other programmers who come along later to use those definitions
> in an incorrect way.
> 
> If it's too much trouble to have the auto-generation mechanism 
> insert the definitions into help.c where they belong, at least
> name the auto-generated file something else, like commands-auto.res
> or command-list.txt.  A #include file's name does not _have_ to
> end in .h, and avoiding the .h convention in a case such as this
> would make it blatantly obvious that something unconventional is
> being done.

I would generally agree.  Nonetheless, I was able to implement
this check without touching any of these files so these should
probably stay as-is.

Thanks,
-- 
David

      reply	other threads:[~2014-09-14  8:45 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-09-14  1:11 [PATCH] help: ensure that common-cmds.h is only included by help.c David Aguilar
2014-09-14  2:00 ` Perry Hutchison
2014-09-14  5:23   ` Junio C Hamano
2014-09-14  7:35     ` David Aguilar
2014-09-14  7:55     ` Perry Hutchison
2014-09-14  8:44       ` David Aguilar [this message]

Reply instructions:

You may reply publicly 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

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

  git send-email \
    --in-reply-to=20140914084429.GA74582@gmail.com \
    --to=davvid@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=perryh@pluto.rain.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.