CMA-ES / c-cmaes

CMA-ES written in ANSI C (yet fairly object-oriented)
Other
56 stars 26 forks source link

Fixed bounds warning (gcc 8.3.0) #25

Closed adamkewley closed 4 years ago

adamkewley commented 4 years ago

Hi, I'm compiling OpenSim and SimBody, which depend on this code. When compiling under gcc 8.3.0, I get the following warning:

c-cmaes/src/cmaes.c: In function ‘szCat’:
c-cmaes/src/cmaes.c:3118:3: warning: ‘strncpy’ specified bound depends on the length of the source argument [-Wstringop-overflow=]
   strncpy ((char *)szBuf, sz1, (unsigned)intMin( (int)strlen(sz1), 698));
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

# multiple times

The code is logically correct, but I think gcc is confused by the usage of intMin? Either way, I looked into the source and initially tried to fix szCat, but it seems to be doing a fair amount of buffer juggling just to concatenate some strings into a static buffer. This proposed change removes szCat, replacing it with a print helper function that should, I hope, have the same side-effects as the original implementation but with slightly simpler code.

I would be very grateful to get this fix--or, at least, some other fix that removes the gcc warning--pulled into this repo, so that those downstream projects (OpenSim/SimBody) can pull the latest c-cmaes, rather than us having to disable warnings, or customize the code.

I can change/modify this PR if you do not like the style or implementation approach, just shoot off a reply and I'll get on it.

nikohansen commented 4 years ago

Thanks for the feedback and contribution! Looks good at first glance. I'll have another more careful look tomorrow before to merge.

adamkewley commented 4 years ago

Ah, great :)

I wasn't sure what the best way to exercise the change was, so I just compiled it - if you know an approach I should use, I could go ahead and give it a whirl.

For the refactored method, I think it's doing what szCat was intended for, but there are differences in the implementation. Again, my main goal here is mostly to swipe away a build warning from OpenSim/SimBody, so I can go and fix this in a less-intrusive way if you think the change is too much.

Thanks again

nikohansen commented 4 years ago

Looks indeed like an improvement to me and I don't see a better fix as well.

adamkewley commented 4 years ago

Great, thank you for this, it's very helpful.

I'll try and get the latest master branch merged into simbody