Re: [PATCH 0/9] Add missing includes and forward declares

From: Jeff King <peff@peff.net>
Date: Aug 11, 2018, 17:34 UTC
Message-ID: <20180811173406.GA9119@sigill.intra.peff.net>
In-Reply-To: <CABPp-BEADR15gOrH+GBQxKLZR2fCQwhaPWgf3VS--Z0bTNP0rA@mail.gmail.com>

On Sat, Aug 11, 2018 at 01:59:50AM -0700, Elijah Newren wrote:

Show 7 quoted lines

> The part of my story you snipped in the ellipsis is kind of important,
> though: "...and decided to determine which header files were missing
> their own necessary #include's and forward declarations."  The way I
> did so was making a simple one-line .c file that included exactly one
> header, and checked to see if I could compile it (without any special
> defines), fixed it up as necessary, then repeated that process for
> every toplevel header.

The rule in Git has always been that your very first include must always be "git-compat-util.h" or an equivalent header that includes it first (like "cache.h"). This is mentioned in CodingGuidelines.

So I think the better test is a two-line .c file with:

  #include "git-compat-util.h"
  #include $header_to_check

And I'd be fine with fixing any compilation failures there, either with forward declarations or recursive includes. I think the in the early days there was some resistance to having a lot of recursive includes, because it _can_ lead to confusion, but IMHO it mostly helps people. And I don't recall much discussion on it in recent years.

Including "git-compat-util.h" in many more headers probably doesn't hurt (after all, it's a noop for every .c file which is following the existing rule). But I'd just as soon not sprinkle it everywhere, nor imply that that people don't need to be including it. It's really important that it comes first because it wants a clean slate and have subtle effects on other header files due to macros. Worse still, some of the effects are platform dependent, so we might not realize an ordering problem until somebody on AIX complains.

I wonder if there's an easy way to check. I guess adding '-Dint=foo' to the command line, and then putting '#undef int' at the top of git-compat-util would catch just about any code the compiler sees that doesn't have git-compat-util included. :)

-Peff