All of lore.kernel.org
 help / color / mirror / Atom feed
From: David Aguilar <davvid@gmail.com>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: git@vger.kernel.org, "Jeff King" <peff@peff.net>,
	"René Scharfe" <l.s.r@web.de>
Subject: Re: [RFC PATCH v2 2/2] headers: include dependent headers
Date: Sun, 7 Sep 2014 13:30:51 -0700	[thread overview]
Message-ID: <20140907203049.GA61326@gmail.com> (raw)
In-Reply-To: <20140907194917.GB1611@google.com>

On Sun, Sep 07, 2014 at 12:49:18PM -0700, Jonathan Nieder wrote:
> David Aguilar wrote:
> 
> > Add dependent headers so that including a header does not
> > require including additional headers.
> 
> I agree with this goal, modulo the compat-util.h caveat.  Thanks
> for working on it.
> 
> [...]
> > --- a/archive.h
> > +++ b/archive.h
> > @@ -1,6 +1,7 @@
> >  #ifndef ARCHIVE_H
> >  #define ARCHIVE_H
> >  
> > +#include "cache.h"
> >  #include "pathspec.h"
> >  
> >  struct archiver_args {
> 
> I'm less happy about the way of achieving that goal.  Here's an
> alternative.  Advantages:
> 
>  * (fully expanded) headers stay small
> 
>  * fewer other headers included as a side-effect of including one
>    header, so callers are more likely to remember to #include the
>    headers defining things they need (which makes later refactoring
>    easier)
> 
>  * circular header dependencies are harder to produce
> 
> If this seems like a good direction to go in, I can finish the patch
> later today

Yes, please, that would be sweet.

Would you mind squashing in your sug to patch 1/2 as well when resending?
It seems like a nice improvement all around.

RE: pre-compiled headers -- that might be a nice follow-up to
this series. I'm not very familiar with Windows so I don't know
if it would be doable on mingw, cygwin, and msvc et al. but if
it helps other platforms then it could be a nice feature.

Thanks Jonathan,
-- 
David

  parent reply	other threads:[~2014-09-07 20:31 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-09-07  0:30 [RFC PATCH v2 1/2] Makefile: add check-headers target David Aguilar
2014-09-07  0:30 ` [RFC PATCH v2 2/2] headers: include dependent headers David Aguilar
2014-09-07  6:41   ` René Scharfe
2014-09-07 19:49   ` Jonathan Nieder
2014-09-07 19:54     ` Johannes Sixt
2014-09-07 20:01       ` Jonathan Nieder
2014-09-07 20:30     ` David Aguilar [this message]
2014-09-07 19:42 ` [RFC PATCH v2 1/2] Makefile: add check-headers target Jonathan Nieder
2014-09-08 18:56 ` Junio C Hamano
2014-09-08 19:29   ` Matthieu Moy
2014-09-08 19:57     ` Junio C Hamano
2014-09-10  0:03       ` David Aguilar
2014-09-10 17:09         ` Junio C Hamano
2014-09-10 17:24           ` Matthieu Moy

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=20140907203049.GA61326@gmail.com \
    --to=davvid@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=jrnieder@gmail.com \
    --cc=l.s.r@web.de \
    --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
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.