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. (4) #1788

Merged
merged 4 commits into from Jan 10, 2015

Conversation

Projects
None yet
6 participants
@elmart
Member

elmart commented Jan 9, 2015

Continues #1715.

Removes 60 long_u instances.
44 still remaining for future PR's.

@marvim marvim added the RFC label Jan 9, 2015

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

This comment has been minimized.

Show comment
Hide comment
@elmart

elmart Jan 9, 2015

Member

Travis build failed because of some warnings issued by gcc that are not issued by clang.
Will fix that and @fwalch suggestion and repush.

Member

elmart commented Jan 9, 2015

Travis build failed because of some warnings issued by gcc that are not issued by clang.
Will fix that and @fwalch suggestion and repush.

@elmart

This comment has been minimized.

Show comment
Hide comment
@elmart

elmart Jan 9, 2015

Member

Fixed and repushed.

Member

elmart commented Jan 9, 2015

Fixed and repushed.

Show outdated Hide outdated src/nvim/hardcopy.c Outdated
(bytes_to_print / 100))
: (int)((prtpos.bytes_printed * 100)
/ bytes_to_print));
assert(prtpos.bytes_printed * 100 > prtpos.bytes_printed);

This comment has been minimized.

@fwalch

fwalch Jan 9, 2015

Member

Won't this always be true?

@fwalch

fwalch Jan 9, 2015

Member

Won't this always be true?

This comment has been minimized.

@melon3r

melon3r Jan 10, 2015

Unless prtpos.bytes_printed is 0...

@melon3r

melon3r Jan 10, 2015

Unless prtpos.bytes_printed is 0...

This comment has been minimized.

@fwalch

fwalch Jan 10, 2015

Member

Right. But I don't really get the initial code either. Is it trying to avoid overflows?

@fwalch

fwalch Jan 10, 2015

Member

Right. But I don't really get the initial code either. Is it trying to avoid overflows?

This comment has been minimized.

@elmart

elmart Jan 10, 2015

Member

It's not always true. That's a way to check for overflow.

@elmart

elmart Jan 10, 2015

Member

It's not always true. That's a way to check for overflow.

This comment has been minimized.

@elmart

elmart Jan 10, 2015

Member

Original code tried to do calculation in two different ways in order to expand the range of values for which calculation could be done without overflow.
Now that prtpos.bytes_printed is size_t, I think range is big enough. So, I simplified calculation to be done just one way, after checking for improbable overflow. I think that's clearer.

@elmart

elmart Jan 10, 2015

Member

Original code tried to do calculation in two different ways in order to expand the range of values for which calculation could be done without overflow.
Now that prtpos.bytes_printed is size_t, I think range is big enough. So, I simplified calculation to be done just one way, after checking for improbable overflow. I think that's clearer.

This comment has been minimized.

@oni-link

oni-link Jan 10, 2015

Contributor

prtpos.bytes_printed starts with value 0, so this assert will be always false for the first loop iteration.

@oni-link

oni-link Jan 10, 2015

Contributor

prtpos.bytes_printed starts with value 0, so this assert will be always false for the first loop iteration.

This comment has been minimized.

@elmart

elmart Jan 10, 2015

Member

Ugh, that's true. I'll change > to >=.
In fact, @gdrooid's comment already implied that, but I didn't realize.

@elmart

elmart Jan 10, 2015

Member

Ugh, that's true. I'll change > to >=.
In fact, @gdrooid's comment already implied that, but I didn't realize.

elmart added some commits Jan 9, 2015

Remove long_u: hardcopy: Refactor long_u.
- <color_related_stuff>: long_u --> uint32_t

  Everywhere long_u was used to hold a color value.
  Color values are supposed to be 32 bits at most.
  Supported architectures have 32 bits ints, so we could have used plain
  ints. But this wouldn't be future-proof, and would be wasteful if a
  future architecture has ints bigger than 32 bits.
  So, uint32_t is perfect to achieve optimal packing no matter the
  architecture.

- bytes_to_print/bytes_printed: long_u --> size_t

  Seems like the correct thing, and gets rid of some casts.
Remove long_u: term: Remove dead code using long_u.
get_long_from_buf() seems not to be used anywhere, and is the only place
where instances of long_u remain at term.c.

@justinmk justinmk added the refactor label Jan 10, 2015

@oni-link

This comment has been minimized.

Show comment
Hide comment
@oni-link

oni-link Jan 10, 2015

We could change the type of member number from long to int in type option_table_T and drop these asserts.

Only two variables and argument table of parse_list_options() are of type option_table_T . printer_opts only uses member number as int and mbfont_opts never uses member number.
Member number for the type option_table_T is only set in parse_list_options() in line 323, so we could check here vor valid range

      table[idx].number = getdigits(&p);       /*advances p*/

or replace getdigitis()(returns long) with a function that returns int.

oni-link commented on src/nvim/hardcopy.c in ed8fbfa Jan 10, 2015

We could change the type of member number from long to int in type option_table_T and drop these asserts.

Only two variables and argument table of parse_list_options() are of type option_table_T . printer_opts only uses member number as int and mbfont_opts never uses member number.
Member number for the type option_table_T is only set in parse_list_options() in line 323, so we could check here vor valid range

      table[idx].number = getdigits(&p);       /*advances p*/

or replace getdigitis()(returns long) with a function that returns int.

This comment has been minimized.

Show comment
Hide comment
@elmart

elmart Jan 10, 2015

Owner

For both cases (changing number and getdigits() return type to int), I had considered doing them in my first run. But, on a second thought, I wasn't sure about them, because:

  • Even if the current code could in fact cope with just an int, so general things as an option which can hold a number, and a routine to get a number from a string, could in fact be useful in future, for something more than an int.
  • Things were starting to cascade too much for these changes to be easily reviewable.

So, I eventually decided to leave those changes out, for a subsequent PR, after some more thought about it.
Now I think it was a wrong decision and I should have done them in the first place.

Owner

elmart replied Jan 10, 2015

For both cases (changing number and getdigits() return type to int), I had considered doing them in my first run. But, on a second thought, I wasn't sure about them, because:

  • Even if the current code could in fact cope with just an int, so general things as an option which can hold a number, and a routine to get a number from a string, could in fact be useful in future, for something more than an int.
  • Things were starting to cascade too much for these changes to be easily reviewable.

So, I eventually decided to leave those changes out, for a subsequent PR, after some more thought about it.
Now I think it was a wrong decision and I should have done them in the first place.

@justinmk

This comment has been minimized.

Show comment
Hide comment
@justinmk

justinmk Jan 10, 2015

Member

The changes to term.c are pretty noisy, but they don't seem to interfere with #1657.

Nice work!

Member

justinmk commented Jan 10, 2015

The changes to term.c are pretty noisy, but they don't seem to interfere with #1657.

Nice work!

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

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

@justinmk justinmk merged commit f2460e9 into neovim:master Jan 10, 2015

1 check passed

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

@justinmk justinmk removed the RFC label Jan 10, 2015

long_u fg_color;
long_u bg_color;
uint32_t fg_color;
uint32_t bg_color;

This comment has been minimized.

@oni-link

oni-link Jan 10, 2015

Contributor

Variable bg_color is not needed. We could directly set pattr->bg_color = PRCOLOR_WHITE at the end of the function.

@oni-link

oni-link Jan 10, 2015

Contributor

Variable bg_color is not needed. We could directly set pattr->bg_color = PRCOLOR_WHITE at the end of the function.

This comment has been minimized.

@elmart

elmart Jan 10, 2015

Member

👍 Good catch.

@elmart

elmart Jan 10, 2015

Member

👍 Good catch.

{
if (fgcol != (long_u)prt_fgcol) {
assert(fgcol <= INT_MAX);
if ((int)fgcol != prt_fgcol) {

This comment has been minimized.

@oni-link

oni-link Jan 10, 2015

Contributor

We could change the type of prt_fgcol from int to uint32_t and drop all casts and range checks.

@oni-link

oni-link Jan 10, 2015

Contributor

We could change the type of prt_fgcol from int to uint32_t and drop all casts and range checks.

This comment has been minimized.

@elmart

elmart Jan 10, 2015

Member

Same as above.

@elmart

elmart Jan 10, 2015

Member

Same as above.

{
assert(bgcol <= INT_MAX);

This comment has been minimized.

@oni-link

oni-link Jan 10, 2015

Contributor

We could change the type of prt_bgcol from int to uint32_t and drop all casts and range checks.

@oni-link

oni-link Jan 10, 2015

Contributor

We could change the type of prt_bgcol from int to uint32_t and drop all casts and range checks.

This comment has been minimized.

@elmart

elmart Jan 10, 2015

Member

Note that I didn't do all possible type refactoring in these files. There are still a lot of things than can be improved, as I just did the strictly necessary to make -Wconversion pass and remove long_u.
This one is a sensible one, though.

@elmart

elmart Jan 10, 2015

Member

Note that I didn't do all possible type refactoring in these files. There are still a lot of things than can be improved, as I just did the strictly necessary to make -Wconversion pass and remove long_u.
This one is a sensible one, though.

@elmart

This comment has been minimized.

Show comment
Hide comment
@elmart

elmart Jan 10, 2015

Member

@oni-link I will address all your comments in a subsequent PR. Thanks!

Member

elmart commented Jan 10, 2015

@oni-link I will address all your comments in a subsequent PR. Thanks!

@elmart elmart deleted the elmart:remove-long_u-4 branch Jan 11, 2015

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

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