Header cleanups #1485
Closed
Header cleanups #1485
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
newren
force-pushed
the
header-cleanups
branch
6 times, most recently
from
23d5eab
to
faeb191
Compare
|
/submit |
|
Submitted as pull.1485.git.1677139521.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
|
User |
|
On the Git mailing list, Derrick Stolee wrote (reply to this): On 2/23/2023 3:05 AM, Elijah Newren via GitGitGadget wrote:
> This series cleans up headers a bit, trying to remove excessive dependency
> on "cache.h". I created this series a while ago, but decided to clean it up
> and submit it due to Emily's recent thread and suggestion that this might be
> helpful to their efforts[1].
Thanks for doing this cleanup. It certainly doesn't look like an easy task.
I read the patches carefully and tried to point out anything that looked
fishy and not fully supported by the commit message. However, some things
are probably only fishy because it was the least-complicated time to make
that change.
I'd be happy with this series as-is, allowing further improvements on top.
I just wanted to poke a little at these bits that were not super-clear in
case there was a possible improvement.
Thanks,
-Stolee |
|
User |
We had several C files ignoring the rule to include one of the appropriate headers first; fix that. While at it, the rule in Documentation/CodingGuidelines about which header to include has also fallen out of sync, so update the wording to mention other allowed headers. Unfortunately, C files in reftable/ don't actually follow the previous or updated rule. If you follow the #include chain in its C files, reftable/system.h _tends_ to be first (i.e. record.c first includes record.h, which first includes basics.h, which first includees system.h), but not always (e.g. publicbasics.c includes another header first that does not include system.h). However, I'm going to punt on making actual changes to the C files in reftable/ since I do not want to risk bringing it out-of-sync with any version being used externally. Signed-off-by: Elijah Newren <newren@gmail.com>
newren
force-pushed
the
header-cleanups
branch
from
faeb191
to
e1450c7
Compare
|
On the Git mailing list, Junio C Hamano wrote (reply to this), regarding adafa65 (outdated): "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Elijah Newren <newren@gmail.com>
>
> Since git-compat-util.h needs to be included first in C files, having it
> appear in header files is unnecessary. More importantly, having it
> included in header files seems to lead to folks leaving it out of C
> files, which makes it harder to verify that the rule is being followed.
> Remove it from header files, other than the ones that have been approved
> as alternate first includes.
Hmph, doesn't this cut both ways?
I like the idea that the removal of compat-util from other
header files may increase the likelihood that a C file that includes
such header files without including compat-util fail to compile,
because it would fail to find what is defined or declared in
compat-util.
But from "include what you use" point of view, shouldn't a header
that defines or declares its own stuff using what is defined or
declared in compat-util be including compat-util itself?
Or do I misunderstand what "make hdr-check" is trying to achieve?
Granted that the check does not fail with this patch in place, but I
suspect that it is by accident (i.e. there happens to be nobody who
depends on what is defined/declared in compat-util for their own
definition or declaration). Also I am not sure how to interpret
the fact that "make hdr-check" succeeds with this patch. Does it
mean C files that include these header files while forgetting to
include compat-util may not be caught by the compiler after all?
So, I dunno.
|
|
On the Git mailing list, Junio C Hamano wrote (reply to this), regarding ec6f251 (outdated): "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Elijah Newren <newren@gmail.com>
>
> This allows us to replace includes of cache.h with includes
> of the much smaller alloc.h in many places.
Yes! This is a very logical thing to do. Allocation and
reallocation, especially in the modern world where "memory is tight,
let's unmap some packfiles" no longer happens, does not have to be
very "git" specific operation and should not have to depend on the
rest of "git". Having to include "cache.h" is one trait I consider
anything is part of the implementation of "git" that has to be
intimate with its internal. |
|
On the Git mailing list, Elijah Newren wrote (reply to this), regarding adafa65 (outdated): On Thu, Feb 23, 2023 at 11:35 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Elijah Newren <newren@gmail.com>
> >
> > Since git-compat-util.h needs to be included first in C files, having it
> > appear in header files is unnecessary. More importantly, having it
> > included in header files seems to lead to folks leaving it out of C
> > files, which makes it harder to verify that the rule is being followed.
> > Remove it from header files, other than the ones that have been approved
> > as alternate first includes.
>
> Hmph, doesn't this cut both ways?
>
> I like the idea that the removal of compat-util from other
> header files may increase the likelihood that a C file that includes
> such header files without including compat-util fail to compile,
> because it would fail to find what is defined or declared in
> compat-util.
>
> But from "include what you use" point of view, shouldn't a header
> that defines or declares its own stuff using what is defined or
> declared in compat-util be including compat-util itself?
>
> Or do I misunderstand what "make hdr-check" is trying to achieve?
>
> Granted that the check does not fail with this patch in place, but I
> suspect that it is by accident (i.e. there happens to be nobody who
> depends on what is defined/declared in compat-util for their own
> definition or declaration). Also I am not sure how to interpret
> the fact that "make hdr-check" succeeds with this patch. Does it
> mean C files that include these header files while forgetting to
> include compat-util may not be caught by the compiler after all?
>
> So, I dunno.
I did something like that before, and Peff objected; see
https://lore.kernel.org/git/20180811173406.GA9119@sigill.intra.peff.net/
and https://lore.kernel.org/git/20180811174301.GA9287@sigill.intra.peff.net/.
I think for sanity we should do one of the following:
(a) make C and header files both depend upon everything they need
(b) consistently exclude git-compat-util.h from headers and require it
be the first include in C files
I think things get really messy if we let half the headers follow (a)
and the other half are forced to do (b). I was pushed towards (b)
before, but now that I've worked on this series, I think there is even
more reason to go this direction: this work I did during this series
shows that if we allow a mixture of (a) and (b), then empirically we
end up with C files that don't include git-compat-util.h directly, and
those same C files likely include some headers that don't include
git-compat-util.h at all, and if the other headers are included before
the indirect inclusion of git-compat-util.h then there are risks that
things will break in very subtle ways (as pointed out by Peff in the
above-linked emails). So, I'm inclined to go towards (b). |
newren
force-pushed
the
header-cleanups
branch
from
e1450c7
to
7694e0f
Compare
|
On the Git mailing list, Junio C Hamano wrote (reply to this), regarding adafa65 (outdated): Elijah Newren <newren@gmail.com> writes:
> I think for sanity we should do one of the following:
>
> (a) make C and header files both depend upon everything they need
> (b) consistently exclude git-compat-util.h from headers and require it
> be the first include in C files
>
> I think things get really messy if we let half the headers follow (a)
> and the other half are forced to do (b). I was pushed towards (b)
> before, but now that I've worked on this series, I think there is even
> more reason to go this direction: this work I did during this series
> shows that if we allow a mixture of (a) and (b), then empirically we
> end up with C files that don't include git-compat-util.h directly, and
> those same C files likely include some headers that don't include
> git-compat-util.h at all, and if the other headers are included before
> the indirect inclusion of git-compat-util.h then there are risks that
> things will break in very subtle ways (as pointed out by Peff in the
> above-linked emails). So, I'm inclined to go towards (b).
Perfect. Can some of that reasoning be captured in the proposed log
message of [02/16] to help future developers?
Thanks. |
For sanity, we should probably do one of the following:
(a) make C and header files both depend upon everything they need
(b) consistently exclude git-compat-util.h from headers and require it
be the first include in C files
Currently, we have some of the headers following (a) and others
following (b), which makes things messy. In the past I was pushed
towards (b), as per [1] and [2]. Further, during this series I
discovered that this mixture empirically will mean that we end up with C
files that do not directly include git-compat-util.h, and do include
headers that don't include git-compat-util.h, with the result that we
likely have headers included before an indirect inclusion of
git-compat-util.h. Since git-compat-util.h has tricky platform-specific
stuff that is meant to be included before everything else, this state of
affairs is risky and may lead to things breaking in subtle ways (and
only on some platforms) as per [1] and [2].
Since including git-compat-util.h in existing header files makes it
harder for us to catch C files that are missing that include, let's
switch to (b) to make the enforcement of this rule easier. Remove the
inclusion of git-compat-util.h from header files other than the ones
that have been approved as alternate first includes.
[1] https://lore.kernel.org/git/20180811173406.GA9119@sigill.intra.peff.net/
[2] https://lore.kernel.org/git/20180811174301.GA9287@sigill.intra.peff.net/
Signed-off-by: Elijah Newren <newren@gmail.com>
We had several header files include cache.h unnecessarily. Remove
those. These have all been verified via both ensuring that
gcc -E $HEADER | grep '"cache.h"'
found no hits and that
cat >temp.c <<EOF &&
#include "git-compat-util.h"
#include "$HEADER"
int main() {}
EOF
gcc -c temp.c
successfully compiles without warnings.
Signed-off-by: Elijah Newren <newren@gmail.com>
We had several C files include cache.h unnecessarily. Replace those
with an include of "git-compat-util.h" instead. Much like the previous
commit, these have all been verified via both ensuring that
gcc -E $SOURCE_FILE | grep '"cache.h"'
found no hits and that
make DEVELOPER=1 ${OBJECT_FILE_FOR_SOURCE_FILE}
successfully compiles without warnings.
Signed-off-by: Elijah Newren <newren@gmail.com>
This allows us to replace includes of cache.h with includes of the much smaller alloc.h in many places. It does mean that we also need to add includes of alloc.h in a number of C files. Signed-off-by: Elijah Newren <newren@gmail.com>
These defines and enum are all oid-related and as such seem to make more sense being included in hash.h. Further, moving them there allows us to remove some includes of cache.h in other files. The change to line-log.h might look unrelated, but line-log.h includes diffcore.h, which previously included cache.h, which included the kitchen sink. Since this patch makes diffcore.h no longer include cache.h, the compiler complains about the 'struct string_list *' function parameter. Add a forward declaration for struct string_list to address this. Signed-off-by: Elijah Newren <newren@gmail.com>
|
On the Git mailing list, Elijah Newren wrote (reply to this): On Fri, Feb 24, 2023 at 4:10 PM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> > This series cleans up headers a bit, trying to remove excessive dependency
> > on "cache.h". I created this series a while ago, but decided to clean it up
> > and submit it due to Emily's recent thread and suggestion that this might be
> > helpful to their efforts[1].
>
> [snip]
>
> > [1] Search for "Extremely yes" in
> > https://lore.kernel.org/git/CAJoAoZm+TkCL0Jpg_qFgKottxbtiG2QOiY0qGrz3-uQy+=waPg@mail.gmail.com/
>
> Thanks, the series looks good.
Thanks for the reviews!
> I especially appreciated the separating
> out of alloc.h and moving function declarations to the .h of the same
> name as the .c file their definition is found in.
There are several more cases where we could do this, but my series was
already long enough and I wanted to get it reviewed (and make sure
others agreed with the directions) before adding more. I'm glad you
like this direction, since I'd really like to move more things this
way. |
|
This patch series was integrated into seen via git@7fd1ef3. |
|
This patch series was integrated into seen via git@df47c5f. |
|
On the Git mailing list, Derrick Stolee wrote (reply to this): On 2/23/2023 7:09 PM, Elijah Newren via GitGitGadget wrote:
> Changes since v1:
>
> * Fixed a bad squash; moved the changes to the right commit
> * More thorough justification in the commit message for the 2nd commit
> * Removed an accidental added include of gettext.h (the file was used, but
> it makes it harder to review that patch)
> * Add some explanation of surprising changes to commit messages
> * Add a new commit which removes the include of hex.h from cache.h, and
> instead makes C files include it directly, as suggested by Stolee
I'm happy with these updates. Thanks for the series!
-Stolee |
|
This patch series was integrated into seen via git@57e66e8. |
|
This patch series was integrated into seen via git@339cd74. |
|
This patch series was integrated into next via git@f4a3707. |
|
This patch series was integrated into seen via git@faddfd2. |
|
This patch series was integrated into seen via git@e381e24. |
|
This patch series was integrated into seen via git@7097f4e. |
|
There was a status update in the "Cooking" section about the branch Code clean-up to clarify the rule that "git-compat-util.h" must be the first to be included. Will cook in 'next'. source: <pull.1485.v2.git.1677197376.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@91619a9. |
|
This patch series was integrated into seen via git@1e1ebca. |
|
This patch series was integrated into seen via git@b700bdb. |
|
This patch series was integrated into seen via git@79dfdb1. |
|
There was a status update in the "Cooking" section about the branch Code clean-up to clarify the rule that "git-compat-util.h" must be the first to be included. Will cook in 'next'. source: <pull.1485.v2.git.1677197376.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@1a2a6b1. |
|
This patch series was integrated into seen via git@b90c2b8. |
|
This patch series was integrated into seen via git@c5248a2. |
|
This patch series was integrated into seen via git@fecd639. |
|
There was a status update in the "Cooking" section about the branch Code clean-up to clarify the rule that "git-compat-util.h" must be the first to be included. Will cook in 'next'. source: <pull.1485.v2.git.1677197376.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@3076712. |
|
This patch series was integrated into seen via git@88cc8ed. |
|
This patch series was integrated into master via git@88cc8ed. |
|
This patch series was integrated into next via git@88cc8ed. |
|
Closed via 88cc8ed. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Maintainer note: This series cleanly merges with next, but has TWO minor conflicts with ed/fsmonitor-inotify. The correct resolution is to take ed/fsmonitor-inotify's copy of both compat/fsmonitor/fsm-settings-darwin.c and compat/fsmonitor/fsm-ipc-darwin.c, but to add a
#include "git-compat-util.h"
line at the top of compat/fsmonitor/fsm-settings-unix.c and a
#include "hex.h"
somewhere near the top of compat/fsmonitor/fsm-ipc-unix.c
Changes since v1:
This series cleans up headers a bit, trying to remove excessive dependency on "cache.h". I created this series a while ago, but decided to clean it up and submit it due to Emily's recent thread and suggestion that this might be helpful to their efforts[1].
There are many more cleanups I could do in this area, but the series is already a good size.
Notes:
[1] Search for "Extremely yes" in https://lore.kernel.org/git/CAJoAoZm+TkCL0Jpg_qFgKottxbtiG2QOiY0qGrz3-uQy+=waPg@mail.gmail.com/
cc: Emily Shaffer nasamuffin@google.com
cc: Derrick Stolee derrickstolee@github.com
cc: Elijah Newren newren@gmail.com
cc: Johannes Schindelin Johannes.Schindelin@gmx.de
cc: Jonathan Tan jonathantanmy@google.com