Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stdint header condition has been reverted. #6020

Conversation

lolgear
Copy link
Contributor

@lolgear lolgear commented Aug 30, 2021

This PR hides header content from non-Windows builds.
( Well, it is better to include this header via "relative" search path ).

@ethomson
Copy link
Member

I'd like @NattyNarwhal's opinion on this.

@NattyNarwhal
Copy link
Contributor

I think it'd still trigger on MSVC regardless; what'd probably be better if the build system checked for stdint's existence and decided to use the reimplementation that way. (My tendency would be an "if stdint.h include stdint.h else reimplementation", and have all of lg2 include git2/stdint.h which does that. OFC, you'd have to make sure the include path never includes include/git2 directly, or you're in include_next hell.)

Which would help non-MS C89 only compilers. (i.e. anyone wanting to port to the othjer really weird/old platforms.)

@ethomson
Copy link
Member

I think it'd still trigger on MSVC regardless; what'd probably be better if the build system checked for stdint's existence and decided to use the reimplementation that way. (My tendency would be an "if stdint.h include stdint.h else reimplementation", and have all of lg2 include git2/stdint.h which does that. OFC, you'd have to make sure the include path never includes include/git2 directly, or you're in include_next hell.)

This is pretty close to the status quo, I think. Right now common.h includes (our) stdint.h iff you're _MSC_VER < 1800. In other words, if we need to define uint64_t so that we can use it in a prototype.

The change here is to remove the error if somebody not on that platform includes this file. I am not super convicted about this error, I guess. People should not be including git2/anything.h directly, they should always just include git2.h. So this was really to prevent people wrongly including git2/stdint.h and now it will just be a noop for them.

I sort of liked the #error, because it is really wrong to include git2/anything.h and this was a good notice for that. I guess. But probably nobody ever included git2/stdint.h (even if they were including things underneath git2/, they were probably never including this one - including git2/merge.h because you want to do a merge makes sense -- including this file doesn't.) So I guess it's not really a particularly useful #error to have around.

I must admit that I'm a little curious about the whole swift thing. I sort of don't understand how swift packages are building us here, because why would they ever hit this error and why do we need to remove it for them? But in this particular instance it doesn't seem so problematic, so this remains more an idle curiosity than a true complaint.

@ethomson
Copy link
Member

Anyway, to make explicit a point that I only alluded to - and one that I always forget about when I see this file and it takes me a while to page this knowledge back in: this file needs to exist so that we can talk about the stdint types publicly (eg, a lot of functions take a size_t and that doesn't exist on MSC_VER < 1800 unless we define it. So that's what we're doing here.

@ethomson ethomson merged commit 1396a9b into libgit2:main Sep 20, 2021
@NattyNarwhal
Copy link
Contributor

I'm pretty sure size_t is defined as part of C89, so you can not worry about that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants