Re: [PATCHv3 1/6] Add missing includes and forward declares

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Aug 15, 2018, 06:13 UTC
Message-ID: <20180815061346.GJ32543@aiede.svl.corp.google.com>
In-Reply-To: <CABPp-BFk2X5TApYzs3QtdokBs3Hqz9uX737M6RGMtaU+wYUikw@mail.gmail.com>

Elijah Newren wrote:

> I didn't want to repeat that description in all 6 patches, since all
> six came from that, so I put it in the cover letter.  Since patch #1
> has most that changes though, I guess it makes sense to include it at
> least in that one?

Yes, that sounds sensible to me.

[...]

> On Tue, Aug 14, 2018 at 10:10 PM Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 7 quoted lines

>> Is there an easy way to review it?  (Keep in mind that I'm super lazy.
>> ;-))
>
> I guess I could send you my hacky python script that loops through the
> top-level header files and creates the dummy two-line c file, and you
> could inspect it and run it.  But that only verifies that it compiles,
> not that the changes I choose are "correct".

I suppose leaving it as an exercise to the reviewer is not terrible. Maybe some reviewer will be inspired to make a test tool we can check in, or configuration for an existing tool like iwyu.

[...]

Show 6 quoted lines

>> enums are of unknown size, so forward declarations don't work for
>> them.  See bb/pedantic for some examples.
>
> structs are also of unknown size; the size is irrelevant when the
> function signature merely uses a pointer to the struct or enum.  The
> enum forward declaration fixes a compilation bug.

My rationale may miss the point but the standard and some real compilers don't like this, unfortunately.

For structs, having an incomplete type is fine, but for enums we need the full definition. E.g. C99 sayeth (in section 6.7.2.3 "tags")

	A type specifier of the form

		enum identifier

	without an enumerator list shall only appear after the type it
	specifies is complete.

[...]

Show 13 quoted lines

>>> --- a/commit-graph.h
>>> +++ b/commit-graph.h
>>> @@ -4,6 +4,7 @@
>>>  #include "git-compat-util.h"
>>>  #include "repository.h"
>>>  #include "string-list.h"
>>> +#include "cache.h"
>>
>> We can skip the #include of git-compat-util.h since all .c files
>> include it.
>
> Good point.  Should I go through and remove all the inclusions of
> git-compat-util.h in header files?

It's orthogonal to this series but might be a good change.

[...]

Show 15 quoted lines

>>> --- a/pathspec.h
>>> +++ b/pathspec.h
>>> @@ -1,6 +1,11 @@
>>>  #ifndef PATHSPEC_H
>>>  #define PATHSPEC_H
>>>
>>> +#include "string.h"
>>> +#include "strings.h"
>>
>> What are these headers?
>
> The original patch[1] had explanations of why I added them:
>
> +#include "string.h"   /* For str[n]cmp */
> +#include "strings.h"  /* For str[n]casecmp */

Ah. Please remove these #includes: they're part of the standard library that we get implicitly via git-compat-util.h.

I was tripped up because they were in quotes instead of angle brackets.

Thanks, Jonathan