Some fixes #115
Some fixes #115
Conversation
When accessing an array element (or, in these instances, characters in a string), the check whether the array index is within bounds should always come before accessing said element. Signed-off-by: Johannes Schindelin <[email protected]>
|
@pks-t would you kindly have a look? |
| @@ -851,7 +851,7 @@ void clar__assert_equal( | |||
| if (!is_equal) { | |||
| if (wcs1 && wcs2) { | |||
| int pos; | |||
| for (pos = 0; wcs1[pos] == wcs2[pos] && pos < len; ++pos) | |||
| for (pos = 0; pos < len && wcs1[pos] == wcs2[pos]; ++pos) | |||
| /* find differing byte offset */; | |||
| p_snprintf(buf, sizeof(buf), "'%.*ls' != '%.*ls' (at byte %d)", | |||
| len, wcs1, len, wcs2, pos); | |||
There was a problem hiding this comment.
This is an obvious improvement indeed.
| char iso_dt[20]; | ||
|
|
||
| if (strftime(iso_dt, sizeof(iso_dt), "%Y-%m-%dT%H:%M:%S", tm) == 0) | ||
| localtime_r(×tamp, &tm); |
There was a problem hiding this comment.
Hm. The change makes sense, but I do wonder whether localtime_r() is even available on all relevant platforms. Our CI indicates that it's not:
D:/a/clar/clar/clar/summary.h:29:9: error: implicit declaration of function 'localtime_r'; did you mean 'localtime_s'? [-Werror=implicit-function-declaration]
29 | localtime_r(×tamp, &tm);
| ^~~~~~~~~~~
| localtime_s
:(
There was a problem hiding this comment.
Good point, Visual C seems to offer localtime_s(), but not localtime_r().
What do you think, should I test for the presence in CMake and then use a conditional #if ... #else ... #endif construct here? Or would you have a different preference?
There was a problem hiding this comment.
What do you think, should I test for the presence in CMake and then use a conditional #if ... #else ... #endif construct here? Or would you have a different preference?
@dscho Ideally it'd work without relying on CMake. We do have an #ifdef _WIN32 block in "clar.c". Can we maybe define a compatibility wrapper or something like that over there?
There was a problem hiding this comment.
@pks-t fixed. Hopefully the CI run passes.
There was a problem hiding this comment.
The `localtime()` function is inherently thread-unsafe and should not be used anymore. Let's use `localtime_r()` instead. As the `localtime_r()` function is not available on Windows, let's add a small shim there that simply calls `localtime_s()`, which is equally thread-safe. Helped-by: Patrick Steinhardt <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
I am playing around with static analyzers, and the first one complained about these two issues, which are easily fixed.