tmolteno / necpp

NEC2++ is a C++ rewrite of the Numerical Electromagnetics Code (NEC-2) with many new features like automatic error detection when you specify the structure incorrectly and much faster execution. Nec2++ can analyse radiating as well as scattering properties of structures. The simulation engine in Nec2++ is compiled into a library for easy integration into automatic antenna design systems or GUI tools. Examples are included for using Nec2++ from C/C++, Ruby and Python.
http://elec.otago.ac.nz/w/index.php/Necpp
GNU General Public License v2.0
239 stars 66 forks source link

`safe_array` indexing should use `size_t` #78

Open StefanBruens opened 1 year ago

StefanBruens commented 1 year ago

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:

[   40s] safe_array.h: In instantiation of 'void safe_array<T>::resize(int64_t) [with T = std::complex<double>; int64_t = long long int]':
[   40s] matrix_algebra.cpp:281:15:   required from here
[   40s] safe_array.h:122:46: error: conversion from 'int64_t' {aka 'long long int'} to 'size_t' {aka 'unsigned int'} may change value [-Werror=conversion]
[   40s]   122 |           std::memcpy(new_data_, _data, _len * sizeof(T));
[   40s]       |                                         ~~~~~^~~~~~~~~~~
GordonLElliott commented 1 year ago

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.

tmolteno commented 1 month ago

I think you're probably correct here. It is a fairly big set of changes, but a pull request would be greatly appreciated.

GordonLElliott commented 1 month ago

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:

1) Fixed the 'size_t' related issues throughout all the code. (More below.) 2) Clean compile in C++20 (which necessarily incorporates 1.)

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.)