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

From: Jeff King <peff@peff.net>
Date: Aug 11, 2018, 17:43 UTC
Message-ID: <20180811174301.GA9287@sigill.intra.peff.net>
In-Reply-To: <20180811043218.31456-2-newren@gmail.com>

On Fri, Aug 10, 2018 at 09:32:10PM -0700, Elijah Newren wrote:

Show 9 quoted lines

> diff --git a/argv-array.h b/argv-array.h
> index a39ba43f57..c46238784c 100644
> --- a/argv-array.h
> +++ b/argv-array.h
> @@ -1,6 +1,8 @@
>  #ifndef ARGV_ARRAY_H
>  #define ARGV_ARRAY_H
>  
> +#include "git-compat-util.h"   /* for LAST_ARG_MUST_BE_NULL */

Following up on what I said in the other message, I think we should omit this. All header files should assume the environment provided by git-compat-util.h is already loaded (and all non-compat C files should make sure they load it first).

Show 9 quoted lines

> diff --git a/color.h b/color.h
> index 5b744e1bc6..74edbffc9d 100644
> --- a/color.h
> +++ b/color.h
> @@ -1,6 +1,8 @@
>  #ifndef COLOR_H
>  #define COLOR_H
>  
> +#include <stdio.h>

This one is more actively wrong than the git-compat-util.h one. Anybody who already included git-compat-util.h would not be helped by this. And anybody who _didn't_ may be hurt, because now git-compat-util.h might run into ordering problems caused by headers already loaded as part of stdio.h (and keep in mind that stdio.h often brings in other system-dependent definitions, too).

Show 9 quoted lines

> diff --git a/bulk-checkin.h b/bulk-checkin.h
> index a85527318b..e036231636 100644
> --- a/bulk-checkin.h
> +++ b/bulk-checkin.h
> @@ -4,6 +4,8 @@
>  #ifndef BULK_CHECKIN_H
>  #define BULK_CHECKIN_H
>  
> +#include "cache.h" /* for object_type */

As I said earlier, I don't mind includes like these that make everybody's lives a little easier, and shouldn't introduce any funny ordering constraints.

IMHO, though, comments like "for X" are probably a bad idea. That may be accurate _now_, but it is very likely to grow stale without anybody noticing. You only care about that "X" if you are thinking about removing the include, and then the right answer is not to trust the comment, but to remove the include and make sure there is no other breakage.

The one exception is if there is some subtlety, like including a header file that uses macros to replace a function, and removing it wouldn't break compilation but produce a different result. But then:

  - I'd hope we don't have anything like that, because it sounds
    horrible ;)

  - you'd probably want a much more explanatory comment

> [...]

The rest of them are all variants on these three (or forward declarations, which I put in the same boat as this third recursive include; i.e., it's a good change).

-Peff