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. (5) #1812

Merged
merged 9 commits into from Jan 19, 2015

Conversation

Projects
None yet
5 participants
@elmart
Member

elmart commented Jan 13, 2015

Continues #1788.

Removes all remaining long_u instances. Finally!

@@ -2204,7 +2205,7 @@ find_ucmd (
eap->cmdidx = CMD_USER;
else
eap->cmdidx = CMD_USER_BUF;
eap->argt = (long)uc->uc_argt;
eap->argt = uc->uc_argt;

This comment has been minimized.

@oni-link

oni-link Jan 15, 2015

Contributor

Member argt of struct exarg has type long. Are we using additional infos why the value cannot be bigger than INT_MAX?

@oni-link

oni-link Jan 15, 2015

Contributor

Member argt of struct exarg has type long. Are we using additional infos why the value cannot be bigger than INT_MAX?

This comment has been minimized.

@elmart

elmart Jan 15, 2015

Member

Note that here we are assigning an uint32_t to a long, not the other way. So, there's no problem.
The variable being assigned to (eap->argt) is of type long.
The value being assigned (uc->uc_argt) is a member of struct ucmd, of type uint32_t. I
Before refactoring (struct ucmd).uc_argt, I checked all values assigned to it never needed more than 4 bytes, and were always positive. Thus I refactored it into an uint32_t. Probably, the same will be true for (struct exarg).argt, but I haven't checked that yet.
This was one of many places were long (and long_u) are used when only 4 bytes are required (which had sense for 16bit platforms, but not for 32bit and above).

What it's true, it's that there's a missing cast here, to avoid warning for mixing signed/unsigned types, which is not being signaled because -Wconversion is not active on ex_docmd.c. The line should be:

eap->argt = (long)uc->uc_argt
@elmart

elmart Jan 15, 2015

Member

Note that here we are assigning an uint32_t to a long, not the other way. So, there's no problem.
The variable being assigned to (eap->argt) is of type long.
The value being assigned (uc->uc_argt) is a member of struct ucmd, of type uint32_t. I
Before refactoring (struct ucmd).uc_argt, I checked all values assigned to it never needed more than 4 bytes, and were always positive. Thus I refactored it into an uint32_t. Probably, the same will be true for (struct exarg).argt, but I haven't checked that yet.
This was one of many places were long (and long_u) are used when only 4 bytes are required (which had sense for 16bit platforms, but not for 32bit and above).

What it's true, it's that there's a missing cast here, to avoid warning for mixing signed/unsigned types, which is not being signaled because -Wconversion is not active on ex_docmd.c. The line should be:

eap->argt = (long)uc->uc_argt

This comment has been minimized.

@elmart

elmart Jan 15, 2015

Member

That last statement is not exactly correct. Let me rephrase:
For 64bit, uint32_t is always safely convertible to long, so everything's correct, even without cast.
For 32bit, warning would be generated, for assigning unsigned to signed of the same size.
Now, we know unsigned value never uses the last bit, so a simple cast to long would suffice.
But that would be fragile, as it would break if new values using the last bit were introduced.
So, proper solution would be to assert assigned value is not bigger than LONG_MAX.
But that could generate and "always true" warning in 64 bits.
So, definitive thing should look like:

assert((uintmax_t)uc->uc_argt <= LONG_MAX);
eap->argt = (long)uc->uc_argt

I will correct to that if you agree.

@elmart

elmart Jan 15, 2015

Member

That last statement is not exactly correct. Let me rephrase:
For 64bit, uint32_t is always safely convertible to long, so everything's correct, even without cast.
For 32bit, warning would be generated, for assigning unsigned to signed of the same size.
Now, we know unsigned value never uses the last bit, so a simple cast to long would suffice.
But that would be fragile, as it would break if new values using the last bit were introduced.
So, proper solution would be to assert assigned value is not bigger than LONG_MAX.
But that could generate and "always true" warning in 64 bits.
So, definitive thing should look like:

assert((uintmax_t)uc->uc_argt <= LONG_MAX);
eap->argt = (long)uc->uc_argt

I will correct to that if you agree.

This comment has been minimized.

@elmart

elmart Jan 15, 2015

Member

Note that this all will disappear once eap->argt is refactored into uint32_t too (if that's possible, as I presume).

@elmart

elmart Jan 15, 2015

Member

Note that this all will disappear once eap->argt is refactored into uint32_t too (if that's possible, as I presume).

This comment has been minimized.

@elmart

elmart Jan 15, 2015

Member

Bad luck.

assert((uintmax_t)uc->uc_argt <= LONG_MAX);

still throws the "always true" warning on 64bits.
I seem not to be able to find an expression that doesn't generate a warning in both 32 and 64bits.
Only solution I have by now is using preprocessor:

#if SIZEOF_LONG <= 4
    assert(uc->uc_argt <= LONG_MAX);
#endif

I haven't needed that until now. Can you think of other option?

@elmart

elmart Jan 15, 2015

Member

Bad luck.

assert((uintmax_t)uc->uc_argt <= LONG_MAX);

still throws the "always true" warning on 64bits.
I seem not to be able to find an expression that doesn't generate a warning in both 32 and 64bits.
Only solution I have by now is using preprocessor:

#if SIZEOF_LONG <= 4
    assert(uc->uc_argt <= LONG_MAX);
#endif

I haven't needed that until now. Can you think of other option?

This comment has been minimized.

@oni-link

oni-link Jan 15, 2015

Contributor

I used grep 'argt\>' src/nvim/* -r to look for (struct exarg).argt. There are only a few places where an assignment is used (=,|=). Only uint32_t values are assigned, so I think it would be easier to just change the type from long to uint32_t for (struct exarg).argt.

@oni-link

oni-link Jan 15, 2015

Contributor

I used grep 'argt\>' src/nvim/* -r to look for (struct exarg).argt. There are only a few places where an assignment is used (=,|=). Only uint32_t values are assigned, so I think it would be easier to just change the type from long to uint32_t for (struct exarg).argt.

This comment has been minimized.

@elmart

elmart Jan 15, 2015

Member

Ok. I'll try that just now. I'll add a new commit (not repush) with the correction when finished, so that you can continue reviewing knowing nothing will change under your feet. I will reintegrate those review commits to their corresponding place when review has finished.

@elmart

elmart Jan 15, 2015

Member

Ok. I'll try that just now. I'll add a new commit (not repush) with the correction when finished, so that you can continue reviewing knowing nothing will change under your feet. I will reintegrate those review commits to their corresponding place when review has finished.

This comment has been minimized.

@elmart

elmart Jan 15, 2015

Member

Done.

@elmart

elmart Jan 15, 2015

Member

Done.

Show outdated Hide outdated src/nvim/misc1.c Outdated
Show outdated Hide outdated src/nvim/option.c Outdated
Show outdated Hide outdated src/nvim/option.c Outdated
Show outdated Hide outdated src/nvim/option.c Outdated
@elmart

This comment has been minimized.

Show comment
Hide comment
@elmart

elmart Jan 16, 2015

Member

Sorry, I had to repush because build was failing for reasons other than this PR.

Member

elmart commented Jan 16, 2015

Sorry, I had to repush because build was failing for reasons other than this PR.

Show outdated Hide outdated src/nvim/option.c Outdated
@elmart

This comment has been minimized.

Show comment
Hide comment
@elmart

elmart Jan 17, 2015

Member

@oni-link Added a couple of review commits fixing asserts in option.c and regexp-nfa.c.
You can continue reviewing when you want.

Member

elmart commented Jan 17, 2015

@oni-link Added a couple of review commits fixing asserts in option.c and regexp-nfa.c.
You can continue reviewing when you want.

@elmart

This comment has been minimized.

Show comment
Hide comment
@elmart

elmart Jan 17, 2015

Member

These kind of asserts are tricky. Even when you have done before, it's easy to make mistakes. So, I'm thinking about the introduction of some macros to check aritmetic operations do not overflow.
Something like: SIGNED_SUM_IN_BOUNDS(operand_1, operand_2, type), that would expand to

(!((operand_2 >= 0 && operand_1 > type_MAX - operand_2)
    || (operand_2 <= 0 && operand_1 < type_MIN - operand_2))))

to be used like

assert(<possibly other conditions>...
       && SIGNED_SUM_IN_BOUNDS(�val1, val2, long));

Same for SIGNED_SUBSTRACTION_IN_BOUNDS, UNSIGNED_SUM_IN_BOUNDS, etc...

That would make much easier for people to insert the correct checkings wherever necessary. And would be much easier to review, too. What do you think?

Member

elmart commented Jan 17, 2015

These kind of asserts are tricky. Even when you have done before, it's easy to make mistakes. So, I'm thinking about the introduction of some macros to check aritmetic operations do not overflow.
Something like: SIGNED_SUM_IN_BOUNDS(operand_1, operand_2, type), that would expand to

(!((operand_2 >= 0 && operand_1 > type_MAX - operand_2)
    || (operand_2 <= 0 && operand_1 < type_MIN - operand_2))))

to be used like

assert(<possibly other conditions>...
       && SIGNED_SUM_IN_BOUNDS(�val1, val2, long));

Same for SIGNED_SUBSTRACTION_IN_BOUNDS, UNSIGNED_SUM_IN_BOUNDS, etc...

That would make much easier for people to insert the correct checkings wherever necessary. And would be much easier to review, too. What do you think?

@oni-link

This comment has been minimized.

Show comment
Hide comment
@oni-link

oni-link Jan 18, 2015

Contributor

That would make much easier for people to insert the correct checkings wherever necessary. And would be much easier to review, too. What do you think?

That would be great.

@oni-link Added a couple of review commits fixing asserts in option.c and regexp-nfa.c.
You can continue reviewing when you want.

If you replace some of the asserts with these new macros I will wait with reviewing the last commits.

Contributor

oni-link commented Jan 18, 2015

That would make much easier for people to insert the correct checkings wherever necessary. And would be much easier to review, too. What do you think?

That would be great.

@oni-link Added a couple of review commits fixing asserts in option.c and regexp-nfa.c.
You can continue reviewing when you want.

If you replace some of the asserts with these new macros I will wait with reviewing the last commits.

@elmart

This comment has been minimized.

Show comment
Hide comment
@elmart

elmart Jan 18, 2015

Member

I'd prefer leaving the macros for a separated PR. I have to think a bit about the optimal shape for them. Also, that way I could review asserts introduced previously, and convert all the suitable ones to use the macros.
For what regards your reviewing this PR, note that the said macros wouldn't be used much in the part you haven't reviewed yet.

Member

elmart commented Jan 18, 2015

I'd prefer leaving the macros for a separated PR. I have to think a bit about the optimal shape for them. Also, that way I could review asserts introduced previously, and convert all the suitable ones to use the macros.
For what regards your reviewing this PR, note that the said macros wouldn't be used much in the part you haven't reviewed yet.

@oni-link

This comment has been minimized.

Show comment
Hide comment
@oni-link

oni-link Jan 19, 2015

Contributor

LGTM.

Contributor

oni-link commented Jan 19, 2015

LGTM.

@elmart

This comment has been minimized.

Show comment
Hide comment
@elmart

elmart Jan 19, 2015

Member

Review commits integrated, and PR rebased.
Thanks so much, @oni-link, for your immense help with those tricky asserts.

Member

elmart commented Jan 19, 2015

Review commits integrated, and PR rebased.
Thanks so much, @oni-link, for your immense help with those tricky asserts.

justinmk added a commit that referenced this pull request Jan 19, 2015

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

@justinmk justinmk merged commit 617c00b into neovim:master Jan 19, 2015

1 check passed

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

@justinmk justinmk removed the RFC label Jan 19, 2015

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Jan 19, 2015

Member

Thank you both. Amazing work.

Member

justinmk commented Jan 19, 2015

Thank you both. Amazing work.

@aktau

This comment has been minimized.

Show comment
Hide comment
@aktau

aktau Jan 19, 2015

Member

Props!

Member

aktau commented Jan 19, 2015

Props!

@elmart elmart deleted the elmart:remove-long_u-5 branch Jan 20, 2015

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