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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

stdintification: use int64_t and INT64_C instead of long long #5941

Merged
merged 6 commits into from
Jul 16, 2021

Conversation

NattyNarwhal
Copy link
Contributor

While both are C99, the type and macro constant can be used in C89 with shims (i.e the one included). long long can't be (and it's a GCC extension), so using the stdint types will be preferable. In addition, we also need to use the macro for constants, because those can differ pre-C99. (As an example, MSVC uses _int64 instead of long long, and i64 as the constant suffix instead of ll.)

Not touched were things in deps/; the NTLM library isn't used on Windows and zlib seems to handle MSVC fine already. I assume the changes can be replicated upstreams and synced anyways.

Tested with gcc 11 on Fedora 34 x64. (As mentioned in the previous comment, I intend to use this with MSVC, but I'm still cleaning up my branch. 馃槵 )

Fixes #5939

Even on systems without C99 where long long and stdint are both
missing, we can shim stdint and point it to any compiler-specific
type (i.e long long, _int64, etc.).

Also next is constant suffixes and determining what needs to
include stdint.
Passes w/ gcc 11 on Fedora x64.

Protip: So you don;t have to suffer,

```
perl -pe 's/(-?(?:0x)?[A-Fa-f0-9]+)([Uu])?[Ll][Ll]/\U$2INT64_C(\E$1)/mg'
```
src/mwindow.c Outdated Show resolved Hide resolved
src/khash.h Outdated Show resolved Hide resolved
lhchavez
lhchavez previously approved these changes Jul 11, 2021
Copy link
Contributor

@lhchavez lhchavez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm in favor of this change \o/

since this change doesn't touch deps/ it might not be feasible at the time, but maybe it's worth adding -Wlong-long to one of the gcc CI builds later if it helps prevent backsliding (i.e. if it complains in all/some of the places that this PR touched). if that doesn't work, maybe adding https://clang.llvm.org/extra/clang-tidy/checks/google-runtime-int.html and #5618 will do the trick.

@ethomson
Copy link
Member

I'm also 馃憤 on this, generally speaking. But I also worry about the long-term drift. Tackling these sort of issues one time is a frustrating experience (and thanks for the endeavor, @NattyNarwhal). But having to revisit them every few months because we aren't catching them in CI is an added frustration that we should avoid.

@lhchavez
Copy link
Contributor

I'm also on this, generally speaking. But I also worry about the long-term drift. Tackling these sort of issues one time is a frustrating experience (and thanks for the endeavor, @NattyNarwhal). But having to revisit them every few months because we aren't catching them in CI is an added frustration that we should avoid.

the flags or clang-tidy should help with this!

For 32-bit int: There's no real reason for that ifdef if we explicitly
specify the stdint.h type.

For 64-bit int: ope, I made it signed accidentally
Copy link
Member

@ethomson ethomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed one more sign change. Otherwise, this is great, thanks!

tests/graph/commit_graph.c Outdated Show resolved Hide resolved
@ethomson ethomson merged commit 6a7f040 into libgit2:main Jul 16, 2021
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.

long long is not standards-compliant C89
3 participants