-
Notifications
You must be signed in to change notification settings - Fork 66
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
safe_array
indexing should use size_t
#78
Comments
I think I successfully changed all int64_t and uint64_t references in the entire program to size_t, in my local copy. (Started with safe_array.) I wanted a clean compile on Windows/Visual Studio 22 using C++20 standard. I think this aspect is working, basic dipole models work. I pretty much examined all the int64_t references and saw none that had any dependency on sign. Doing this I get completely clean compiles in 32 bit as well as 64 bit modes (but many other changes were needed of course for that absolutely clean compile). I note the sign issue because size_t is unsigned, so changing from a signed to unsigned integer to do the above. I have some flow control issues, but that I don't believe has any relation to the above. |
I think you're probably correct here. It is a fairly big set of changes, but a pull request would be greatly appreciated. |
Ah, saw this by email -- after more than a year ago. (I have not used this for over a year, though will return to a project some day needing the software.) There are two subheadings in what I did, and everyone should separate those carefully as for most part different projects:
My recollection is that what I said should be completely safe regarding 1) 'size_t' issue. I carefully analyzed, and all occurrences of any 64 bit integers in the entire program were only using positive values and were clearly used for indexing purposes, so unsigned 'size_t' was going to work in every instance. I see there was a merge/update of some sort since my looking. I would guess that is still going to be the case. I've mostly worked with others on open source like on Github by making recommendations and I have not learned to use the tools, so would be a bit before I was even ready to do pull, etc. And I'm stacked with several projects and behind on all, so will be months to even get to that priority. Only my '1)' is directly related to this issue, and I think that just changing every instance of 64 bit variables to size_t would be very safe. I don't quite understand the testing the community would feel needed, obviously beyond my Microsoft compilers. The main point here is that I didn't just change and test those instances, but carefully looked at the code in each instance to assure it was going to work, so I personally think this is a very safe change and anyone can do that. On my '2)' subheadings, that is a much larger change, and is outside of the scope of this issue, though vaguely related as an encompassing desire that would include this issue. I would feel very uncomfortable with all the changes I made to get a truly clean compile on C++20 without extensive testing on a large range of compilers, something I won't have time to do this year. I'm not even sure what the range of C++ versions the community is using, or the lowest level that is to be supported. (Even though I am very confident that my version works as well as any copy.) |
size_t is the index type used for all containers in the STL.
It is a 64 bit integer on 64 bit archs, and 32 bit on 32 bit archs.
Using int64_t breaks compilation on 32 bit archs:
The text was updated successfully, but these errors were encountered: