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

[RDY] Fix width of long_u and friends. #296

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
4 participants
@mahkoh
Copy link
Contributor

commented Mar 5, 2014

This might break printf and scanf on Windows for now. See the comment in vim.h in the diff.

@tarruda

This comment has been minimized.

Copy link
Member

commented Mar 6, 2014

I'm ok with it, as mentioned in #287 we can normalize this for windows later.

@mahkoh If your'e interested, I also prefer using c99 stdbool.h instead of those TRUE/FALSE macros, in fact I'm already using on my feature branch.

There's just one change I think its necessary, we should use explicit includes in all files, so any files that use uint32_t/bool should include the necessary headers.(I will fix my own branch to reflect that)

@mahkoh

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2014

@tarruda: Replacing TRUE/FALSE seems natural but there is also this macro in vim.h:

#define MAYBE   2           /* sometimes used for a variant on TRUE */

ack MAYBE | wc -l
55

Since TRUE/true and FALSE/false cast, refactoring the functions that don't use MAYBE should be trivial. But we'll have to take great care not to break the code that stores MAYBE in structs or global variables.

@mahkoh

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2014

@tarruda: I've added the includes.

@tarruda

This comment has been minimized.

Copy link
Member

commented Mar 7, 2014

@mahkoh I've fixed some problems with valgrind configuration in #321 , can you rebase so travis will review your PR?

@tarruda

This comment has been minimized.

Copy link
Member

commented Mar 7, 2014

Here's what I get when building locally:

[  0%] Building C object src/CMakeFiles/nvim.dir/hardcopy.c.o
[  1%] Building C object src/CMakeFiles/nvim.dir/file_search.c.o
[  2%] Building C object src/CMakeFiles/nvim.dir/os_unix.c.o
[  3%] Building C object src/CMakeFiles/nvim.dir/mbyte.c.o
[  4%] Building C object src/CMakeFiles/nvim.dir/if_cscope.c.o
[  5%] Building C object src/CMakeFiles/nvim.dir/getchar.c.o
[  6%] Building C object src/CMakeFiles/nvim.dir/fileio.c.o
[  7%] Building C object src/CMakeFiles/nvim.dir/edit.c.o
[  8%] Building C object src/CMakeFiles/nvim.dir/ex_cmds.c.o
/home/tarruda/neovim/src/ex_cmds.c: In function ‘ex_sort’:
/home/tarruda/neovim/src/ex_cmds.c:466:9: error: overflow in implicit constant conversion [-Werror=overflow]
/home/tarruda/neovim/src/ex_cmds.c: In function ‘do_filter’:
/home/tarruda/neovim/src/ex_cmds.c:1131:11: error: overflow in implicit constant conversion [-Werror=overflow]
cc1: all warnings being treated as errors
m

I wonder why travis didn't caught this. To tired to think about right now, tomorrow I will take a closer look

@mahkoh

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2014

You're using a 32 bit system?

Edit: I'll change INT64_MAX to INT32_MAX for now. We can change this later when we've removed all long from the source so that users can have files with more than 4 billion lines.

@tarruda

This comment has been minimized.

Copy link
Member

commented Mar 8, 2014

Yes, I develop on a 32-bit linux virtual machine

Even after your last update I'm still getting these problems:

memline.c:4292:13: error: conflicting types for ‘ml_updatechunk’
memline.c:274:13: note: previous declaration of ‘ml_updatechunk’ was here
memline.c:274:13: error: ‘ml_updatechunk’ used but never defined [-Werror]
memline.c:4292:13: error: ‘ml_updatechunk’ defined but not used [-Werror=unused-function]

ex_docmd.c: In function ‘makeopens’:
ex_docmd.c:8285:15: error: format ‘%ld’ expects argument of type ‘long int’, but argument 3 has type ‘linenr_T’ [-Werror=format]

spell.c: In function ‘node_compress’:
spell.c:7070:37: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]

mark.c: In function ‘show_one_mark’:
mark.c:686:7: error: format ‘%ld’ expects argument of type ‘long int’, but argument 4 has type ‘linenr_T’ [-Werror=format]

mark.c: In function ‘ex_jumps’:
mark.c:802:11: error: format ‘%ld’ expects argument of type ‘long int’, but argument 5 has type ‘linenr_T’ [-Werror=format]

mark.c: In function ‘copy_viminfo_marks’:
mark.c:1489:11: error: format ‘%ld’ expects argument of type ‘long int *’, but argument 3 has type ‘linenr_T *’ [-Werror=format]

search.c: In function ‘show_pat_in_path’:
search.c:4581:7: error: format ‘%ld’ expects argument of type ‘long int’, but argument 3 has type ‘linenr_T’ [-Werror=format]

option.c: In function ‘set_init_1’:
option.c:2042:46: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
option.c:2049:48: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]

option.c: In function ‘set_option_default’:
option.c:2278:31: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
option.c:2287:33: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]

option.c: In function ‘set_number_default’:
option.c:2359:44: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]

option.c: In function ‘set_title_defaults’:
option.c:2598:41: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
option.c:2604:41: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]

option.c: In function ‘do_set’:
option.c:2897:32: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
option.c:2944:29: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]

option.c: In function ‘optval_default’:
option.c:6265:35: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]
option.c:6269:39: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast]

tag.c: In function ‘do_tags’:
tag.c:983:11: error: format ‘%ld’ expects argument of type ‘long int’, but argument 7 has type ‘linenr_T’ [-Werror=format]

quickfix.c: In function ‘qf_list’:
quickfix.c:1811:9: error: format ‘%ld’ expects argument of type ‘long int’, but argument 3 has type ‘linenr_T’ [-Werror=format]
quickfix.c:1814:13: error: format ‘%ld’ expects argument of type ‘long int’, but argument 3 has type ‘linenr_T’ [-Werror=format]

quickfix.c: In function ‘qf_fill_buffer’:
quickfix.c:2355:9: error: format ‘%ld’ expects argument of type ‘long int’, but argument 3 has type ‘linenr_T’ [-Werror=format]

fold.c: In function ‘put_folds_recurse’:
fold.c:2953:13: error: format ‘%ld’ expects argument of type ‘long int’, but argument 3 has type ‘linenr_T’ [-Werror=format]
fold.c:2953:13: error: format ‘%ld’ expects argument of type ‘long int’, but argument 4 has type ‘linenr_T’ [-Werror=format]

fold.c: In function ‘put_foldopen_recurse’:
fold.c:2977:9: error: format ‘%ld’ expects argument of type ‘long int’, but argument 3 has type ‘linenr_T’ [-Werror=format]

fold.c: In function ‘put_fold_open_close’:
fold.c:3014:3: error: format ‘%ld’ expects argument of type ‘long int’, but argument 3 has type ‘linenr_T’ [-Werror=format]
@mahkoh

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2014

Well, these are the problems I anticipated on Windows. The thought that anyone could be using a 32 bit UNIX didn't cross my mind.

I'll try to fix this.

@mahkoh

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2014

So, to make this work on 32 bit a lot of refactoring has to be done. And one problem is that it has to happen all at once. With the amount of work that's currently going on and the state of the code, doing this now would be entirely unreasonable.

I'll close this and #300 for now.

@mahkoh mahkoh closed this Mar 8, 2014

@philix

This comment has been minimized.

Copy link
Member

commented Mar 12, 2014

libuv has ./third-party/libuv/include/stdint-msvc2008.h which allow us to use stdint on Windows. What's preventing us from using it?

@mahkoh

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2014

The problem is that long is 32 bit on windows.

@philix

This comment has been minimized.

Copy link
Member

commented Mar 12, 2014

@mahkoh yeah, but ./third-party/libuv/include/stdint-msvc2008.h defines uint64_t correctly. We can typedef long_u and long_i to be uint64_t and int64_t. Long being 32-bit on Windows is easily solved by using only long_u and long_i then.

stdint-msvc2008.h correctly uses __w64 [1] which is not the case in vim.h now.

[1] http://msdn.microsoft.com/en-us/library/s04b5w00.aspx

@mahkoh

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2014

printf("%ld", (int64_t)0);

is a bug on Windows.

ack "%ld" | wc -l
4752
@justinmk

This comment has been minimized.

Copy link
Member

commented Mar 12, 2014

@mahkoh Can we just change those occurrences to use the PRIu64 macro in <inttypes.h>? I'm willing to do this if no one wants to and we think it is a good idea.

printf("%" PRIu64, (uint64_t)0);

http://msinttypes.googlecode.com/svn/trunk/inttypes.h

http://stackoverflow.com/questions/9225567/how-to-print-a-int64-t-type-in-c#comment26663428_9225643

http://stackoverflow.com/a/9225648/152142

@philix

This comment has been minimized.

Copy link
Member

commented Mar 12, 2014

@mahkoh I see. That's the reason you closed this PR. We can start using the fixed width types gradually.

By having a portable stdint inclusion on types.h we can use fixed-width types directly for local variables that are not being printed or refactor the printing code when possible. We should actually leave long_u as is while we migrate towards a codebase free of project specific integer types. We have to agree that fixed-width will be used instead of long_u and long_i. IMO uint64_t and friends are much better than project defined types for code clarity and maintenance.

@philix

This comment has been minimized.

Copy link
Member

commented Mar 12, 2014

@justinmk refactoring all occurrences now would be risky as many other refactorings are being performed. To be fair, long_u is supposed to be unsigned long no matter which is the size of long in the platform. Forcing it to be 64-bit on Windows complicates things even further. The C99 standard only requires long to be at least 32-bits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.