vstring: Avoid int -> char truncation warnings #3690
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #3690 +/- ##
=======================================
Coverage 82.99% 83.00%
=======================================
Files 226 226
Lines 55006 55013 +7
=======================================
+ Hits 45652 45662 +10
+ Misses 9354 9351 -3
☔ View full report in Codecov by Sentry. |
|
Sorry for the late response. I will look into this one next. |
|
@b4n, sorry for being late. |
|
Ok, I'll try to do that in the coming days
Le 12 mai 2023 19:20:50 GMT+02:00, Masatake YAMATO ***@***.***> a écrit :
…
@masatake commented on this pull request.
> @@ -106,7 +106,7 @@ CTAGS_INLINE void vStringPut (vString *const string, const int c)
if (string->length + 1 == string->size) /* check for buffer overflow */
vStringResize (string, string->size * 2);
- string->buffer [string->length] = c;
+ string->buffer [string->length] = (char) c;
I agree with you.
Could you add the change you proposed to this pull request?
--
Reply to this email directly or view it on GitHub:
#3690 (comment)
You are receiving this because you authored the thread.
Message ID: ***@***.***>
|
|
@b4n Thank you. Please, delete my commit appended to your branch (if you need). |
Either avoid or silence expected casts from int to char. I'm not entirely sure whether vStringPut() shouldn't actually take a char argument instead of an int, as it will cast it in all cases. It is however slightly more convenient as the APIs to retrieve characters uses int only for edge cases (e.g. EOF), so the common case is a safe cast and it's more convenient not to explicit it everywhere.
|
OK, this blew widely out of proportions, but I verified and fixed all1 calls to Footnotes
|
We expect an `unsigned char` passed as an `int`. Anything else is invalid and probably means there is an issue in the calling code. Co-Authored-By: Masatake YAMATO <yamato@redhat.com>
|
|
||
| d [i] = tolower (c); | ||
| } | ||
| d [i] = (char) tolower (s [i]); |
There was a problem hiding this comment.
Your change itself looks good to me.
Following the consideration that I made during reviewing is not directly related to this pull request. If I wrote incorrect things, please, let me know.
s [i] can be a negative if char on this platform means signed char.
In tolower(3) man page:
If c is neither an unsigned char value nor EOF, the behavior of these functions is undefined.
So tolower (s [i]) can be a trouble. Am I wrong?
If we declare s as unsigned char *, and d as unsigned char *,
we can avoid the trouble.
There was a problem hiding this comment.
8fffa20 cares what I feared here.
|
Yes, i have a changeset almost ready going over most of the ctype calls to clean them up. Hopefully I'll submit it today still, no guarantee :)
Le 15 mai 2023 18:59:35 GMT+02:00, Masatake YAMATO ***@***.***> a écrit :
…
@masatake commented on this pull request.
> @@ -244,11 +244,7 @@ extern void vStringCopyToLower (vString *const dest, const vString *const src)
vStringResize (dest, src->size);
d = dest->buffer;
for (i = 0 ; i < length ; ++i)
- {
- int c = s [i];
-
- d [i] = tolower (c);
- }
+ d [i] = (char) tolower (s [i]);
Your change itself looks good to me.
Following the consideration that I made during reviewing is not directly related to this pull request. If I wrote incorrect things, please, let me know.
`s [i]` can be a negative if `char` on this platform means `signed char`.
In `tolower(3)` man page:
If c is neither an unsigned char value nor EOF, the behavior of these functions is undefined.
So `tolower (s [i])` can be a trouble. Am I wrong?
If we declare s as `unsigned char *`, and d as `unsigned char *`,
we can avoid the trouble.
--
Reply to this email directly or view it on GitHub:
#3690 (review)
You are receiving this because you were mentioned.
Message ID: ***@***.***>
|
|
The commit, vstring: Avoid int -> char truncation warnings should be merged. The rest commits are optional. @b4n, have you ever thought about _Generic of C11? This looks fit on our demand. The biggest issue is we have to move to C11. I also wrote a short program to understand _Generic: |
No, but admittedly I never took time to get into C11 much, mostly because it was too recent for project I worked on. But indeed, it looks interesting here if we can depend on it (e.g. not if it's conditional, because then callers either have to be correct anyway, or some builds will be broken and some will not). In practice it could be as simple as something like this (with #define vStringPut(s, c) _Generic((c), \
char: vStringPutImpl((s), (unsigned char) (c)), \
signed char: vStringPutImpl((s), (unsigned char) (c)), \
default: vStringPutImpl((s), (c)))
This said, I didn't test this and I don't know if it's reasonable to depend on In our case though, I wonder if we couldn't simply do a tiny hack without C11: #define vStringPut(s, c) (sizeof(c) == sizeof(char) \
? vStringPutImpl((s), (unsigned char) (c)) \
: vStringPutImpl((s), (c)))What do you think? |
|
@b4n looks nice! _Generic is interesting but we don't need now. |
vStringPut() and friends take an `unsigned char` as an `int`, similar to ctype functions, in order to be easy to use with `fgetc()`-style functions. However, this API is also used on regular C strings, which require a cast if the value is larger than 0x7F (127) on systems where `char` is a signed type. In order to make the API easier to use, as it's easy to forget the cast when working with `char`, introduce wrapper macros that add the cast when called with `char`. The cast is conditional so the underlying implementation can still verify the value is in the valid range when called with an `int` (to catch erroneous calls with EOF or other values that do not fit a `char`). Note that this will still not work properly if the caller has an incorrect cast on its side, like e.g. `vStringPut(s, (int) c)` where `c` is a `char`, as there's only so many magic tricks up our sleeves. These calls should be updated to either be `vStringPut(s, c)` with the added macros, or `vStringPut(s, (unsigned char) c)` if one wants to be explicit -- yet no need to go all the trouble to make them `vStringPut(s, (int) (unsigned char) c)`, as the `unsigned char` to `int` conversion is implicit and safe. Based off a suggestion from Masatake YAMATO <yamato@redhat.com>
|
I updated the patchset to move the commit making
Well, to be fair I didn't think about it right away, but playing a little with |
|
Wait, apparently I messed up something in the rebase, hang on. |
`vStringPut()` has been made forgiving in the type the character is pass as in e6e018b. It however requires the value to be valid in one of the formats it accepts. This commits should update all problematic cases, basically removing all explicit casts: * removing incorrect `int` to `char` casts; * removing incorrect `char` to `int` casts; * removing unnecessary `char` to `unsigned char` casts. * removing unnecessary `unsigned char` to `int` casts. Incorrect casts can lead to unexpected results with values > 0x7F. Unnecessary casts make the code harder to read than necessary, and risk being incorrectly copied to code where they would be incorrect.
`cppGetc()` can return `CHAR_SYMBOL` and `STRING_SYMBOL` which need to be properly handled when trying to represent as character.
|
Fixed, I removed too many things from the "fix calls" commit, sorry. Should be all good now, and I reviewed them more carefully again. |
|
LGTM. Thank you very much. |
Either avoid or silence expected casts from int to char.
I'm not entirely sure whether vStringPut() shouldn't actually take a char argument instead of an int, as it will cast it in all cases. It is however slightly more convenient as the APIs to retrieve characters uses int only for edge cases (e.g. EOF), so the common case is a safe cast and it's more convenient not to explicit it everywhere.