Fix compile errors and warnings on some compilers #33
Conversation
|
Review per commit is a bit tricky but that's what I'll try to do |
rieder
left a comment
rieder
left a comment
There was a problem hiding this comment.
I have a few questions but generally no weird stuff, ok to merge I think.
NB: did not try to compile/run it yet.
| * make_profile -- make profile for whole n-binary cluster | ||
| *----------------------------------------------------------------------------- | ||
| */ | ||
| /* Disabled because make_profile in double_support.C is broken |
There was a problem hiding this comment.
does this need to be fixed? or can we leave it out?
There was a problem hiding this comment.
Well, it requires something that's broken/disabled, so if it were used then we would have noticed that already, hopefully. SeBa simulates single stars right? So then a function for making a cluster is probably not needed.
| * make_profile -- | ||
| *----------------------------------------------------------------------------- | ||
| */ | ||
| /* Disabled because get_new_binary returns NULL, so this is broken |
There was a problem hiding this comment.
If neither of us know, is there anyone else who can answer that?
There was a problem hiding this comment.
was this a duplicate with a wrong name?
There was a problem hiding this comment.
No, I think that the embedded starlab is only a partial copy, some (presumably unused) parts of it seem to have been removed, or were never copied in. It seems like the build system clean-up causes it to now try to build some things that weren't built before, and with parts of starlab missing, they're erroring out. Since SeBa doesn't use this anyway, I thought the easiest solution was just to remove it.
| // Not very efficiently written, but... (Steve, 12/98) | ||
|
|
||
| static char new_name[1024]; | ||
| static char new_name[1040]; |
There was a problem hiding this comment.
why 1040? seems a bit random to increase by 16...
There was a problem hiding this comment.
The compiler does the math and tells you how much space is needed. I think in this case it's 1024 for the string, 11 for a 32-bit int plus sign, 2 more for the < and >, and finally the terminating NULL, which adds up to 1040.
Of course in practice the string will probably be shorter than 1024 characters, but unless we switch to dynamically allocated strings everywhere, this is what we get.
2 participants
This fixes a whole bunch of warnings (some of which are errors on some compilers), mostly in the included Starlab which is pre-standardisation C++. For me it now compiles without warnings on GCC 13.3.0 (from conda), GCC 11.4.0 (Ubuntu) and Clang 14 (Ubuntu).
Some of the fixes for conversions make things a bit messy because C++ insists on using unsigned int (std::size_t) for sizes and offsets, and that ends up spreading throughout your whole code base and messing things up unless you add some ugly casts. They should have made the signedness implementation-defined, with a compiler switch (because there are cases where it's useful). But well, I've changed types and added casts as appropriately as I could.
This also fixes another thing, which is the use of the
-DTOOLBOXswitch. This was passed everywhere, but as far as I can see the idea is to use it when compiling executables, but not when compiling libraries. So I've changed it to do that, because I was getting linking errors. Another linking error was due to a missing C++ library, the Makefile relies on the implicit rule for linking, and that uses the C compiler, not the C++ compiler, so we don't get the C++ standard library.I've checked that the first command in README.md produces the same output file as in
main, so this shouldn't have broken anything.Note that AMUSE was already patching the main Makefile to remove the
-DTOOLBOX, so that patch can be removed there as well when we upgrade.Best reviewed per commit, they're somewhat logical.
Closes #32.