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

[RFC] Remove project-specific integer types: long_u. (3) #1715

Merged
merged 3 commits into from Dec 24, 2014

Conversation

Projects
None yet
3 participants
@elmart
Member

elmart commented Dec 22, 2014

Continues #1340.

Removes 57 long_u instances.
104 still remaining for future PR's.

@marvim marvim added the RFC label Dec 22, 2014

@justinmk justinmk added the refactor label Dec 22, 2014

@@ -1121,7 +1111,7 @@ void u_write_undo(char_u *name, int forceit, buf_T *buf, char_u *hash)
&& os_fileinfo((char *)buf->b_ffname, &file_info_old)
&& os_fileinfo((char *)file_name, &file_info_new)
&& file_info_old.stat.st_gid != file_info_new.stat.st_gid
&& os_fchown(fd, -1, file_info_old.stat.st_gid) != 0) {
&& os_fchown(fd, (uv_uid_t)-1, (uv_gid_t)file_info_old.stat.st_gid)) {

This comment has been minimized.

@justinmk

justinmk Dec 24, 2014

Member

I wonder why libuv's own uv_stat_t.st_gid is defined as uint64_t. Should we just change the signature of os_fchown to accept uint64_t? cc @stefan991

@justinmk

justinmk Dec 24, 2014

Member

I wonder why libuv's own uv_stat_t.st_gid is defined as uint64_t. Should we just change the signature of os_fchown to accept uint64_t? cc @stefan991

This comment has been minimized.

@justinmk

justinmk Dec 24, 2014

Member

Also, uv_uid_t resolves to unsigned int. Shouldn't we change -1 to UINT_MAX or is (unsigned int)-1 an acceptable C idiom?

@justinmk

justinmk Dec 24, 2014

Member

Also, uv_uid_t resolves to unsigned int. Shouldn't we change -1 to UINT_MAX or is (unsigned int)-1 an acceptable C idiom?

This comment has been minimized.

@elmart

elmart Dec 24, 2014

Member

I wonder why libuv's own uv_stat_t.st_gid is defined as uint64_t.

Me too. Could be a small inconsistency. But haven't really looked into it.

Should we just change the signature of os_fchown to accept uint64_t?

Could be an option.

Also, uv_uid_t resolves to unsigned int. Shouldn't we change -1 to UINT_MAX or is (unsigned int)-1 an acceptable C idiom?

The fact that uv_uid_t resolves to unsigned int should remain hidden, so that it can be changed. So I don't like relying on UINT_MAX. The ideal thing should be having a constant UV_UID_MAX. Lacking that, I don't think the (uv_uid_t)-1 idiom is bad, as that would automatically adapt to changes.

@elmart

elmart Dec 24, 2014

Member

I wonder why libuv's own uv_stat_t.st_gid is defined as uint64_t.

Me too. Could be a small inconsistency. But haven't really looked into it.

Should we just change the signature of os_fchown to accept uint64_t?

Could be an option.

Also, uv_uid_t resolves to unsigned int. Shouldn't we change -1 to UINT_MAX or is (unsigned int)-1 an acceptable C idiom?

The fact that uv_uid_t resolves to unsigned int should remain hidden, so that it can be changed. So I don't like relying on UINT_MAX. The ideal thing should be having a constant UV_UID_MAX. Lacking that, I don't think the (uv_uid_t)-1 idiom is bad, as that would automatically adapt to changes.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Dec 24, 2014

Member

Impeccable as usual, LGTM.

Might want to rebase this since #1493 was merged. Just in case.

Member

justinmk commented Dec 24, 2014

Impeccable as usual, LGTM.

Might want to rebase this since #1493 was merged. Just in case.

elmart added some commits Dec 21, 2014

Remove long_u: put_bytes(): Refactor.
Remove all long_u instances due to put_bytes() function.

First, function signature is changed this way:
- nr  : long_u --> uintmax_t
    uintmax_t is chosen so that invocations can use any unsigned integer
    type (including size_t) without needing to cast.
- len : int    --> unsigned int
    This is to pass the size in bytes of the previous param, thus an
    unsigned int is enough. All invocations use positive integer
    literals, so change is safe without the need for casts.

Then, function implementation is adapted accordingly.

Last, all invocation points are refactored this way:
- Refactor types to minimize casts.
- Inline declarations (C99 style) in containing function.

All this changes were done with -Wconversion temporarily activated for
spell.c and undo.c, so that we can assert changes are type-safe and do
not introduce any warnings to that respect.
Remove long_u: Passing-by: undo.c: Add to -Wconversion.
Previous commit dropped many -Wconversion warnings in both spell.c and
undo.c. spell.c still has a lot of them (200+). But in undo.c, only a
handful of them remain. Take the chance to eliminate those, too, and add
undo.c to -Wconversion checked files.
Remove long_u: Passing-by: put_time(): Refactor implementation.
put_time() had a complicated implementation, because of having to shift
an 8-byte value in a portable way with old means.
That can be greatly simplified now, using a C99 fixed-size type.
@elmart

This comment has been minimized.

Show comment
Hide comment
@elmart

elmart Dec 24, 2014

Member

Rebased.

Member

elmart commented Dec 24, 2014

Rebased.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Dec 24, 2014

Member

Oh boy, looks like I messed up the deps. Sigh Fixed

Member

justinmk commented Dec 24, 2014

Oh boy, looks like I messed up the deps. Sigh Fixed

justinmk added a commit that referenced this pull request Dec 24, 2014

Merge pull request #1715 from elmart/remove-long_u-3
Remove project-specific integer types: long_u. (3)

@justinmk justinmk merged commit f13b90c into neovim:master Dec 24, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@justinmk justinmk removed the RFC label Dec 24, 2014

@elmart elmart deleted the elmart:remove-long_u-3 branch Dec 26, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment