Small steps towards enabling -Wsign-conversion for some submodules #3413
Conversation
felipecrv
force-pushed
the
sign_conversion
branch
from
a7a036f
to
af73dcd
Compare
Doing `return std::move(value)` is, in general, not a good idea, and GCC
warns about it.
```
src/libponyc/codegen/genjit.cc: In lambda function:
src/libponyc/codegen/genjit.cc:46:34: error: redundant move in return
statement [-Werror=redundant-move]
46 | if (err) return std::move(err);
| ~~~~~~~~~^~~~~
src/libponyc/codegen/genjit.cc:46:34: note: remove ‘std::move’ call
```
But in this case, this warning is a false-positive. What's happening is
an implicit conversion from `Error &&` to `JITSymbol`. Changing the code
to be explicit rather than implicit fixes the warning and makes the code
more clear.
felipecrv
force-pushed
the
sign_conversion
branch
from
6deb361
to
7d0245b
Compare
felipecrv
changed the title
felipecrv
changed the title
|
@SeanTAllen my initial plans were more ambitious, but I decided to reduce the scope of this PR to contain only these initial steps. Please review whenever you can. |
|
One approach that might work here is to encapsulate these casts in macros so that they could be switch on or off based on platform or compiler flags. They could also be easier to locate and change as needed. |
|
@cquinn making those dependent on compilation macros might hide some bugs. And the point of explicit casts is to reduce the risk of bugs due to implicit conversions. Most of the explicit casts wouldn't be needed if the code was re-structured in a way that is more portable. The explicit casts work as a reminder of that. |
|
True, but I think the macros would be at least as visible as the casts. The discussion on the dev sync brought up that this change actually makes these conversions harder to find than just leaving them implicit and letting the compiler give us a warning/error. The macro idea was to make these casts stand out for later fixing, and also allow their internals to be easily modified. |
|
I see. That is a good point.
Perhaps my approach here should be about adding only explicit casts that we
intend to keep and re-structure the code in casts that we don't intent to
keep. An example of the former is the list index function and an example of
the latter is casts like `(char)-1`.
…On Tue, 3 Dec 2019 at 22:56, Carl Quinn ***@***.***> wrote:
True, but I think the macros would be at least as visible as the casts.
The discussion on the dev sync brought up that this change actually makes
these conversions harder to find than just leaving them implicit and
letting the compiler give us a warning/error. The macro idea was to make
these casts stand out for later fixing, and also allow their internals to
be easily modified.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#3413?email_source=notifications&email_token=AABSXMYIQFWZIVND3YYCWLTQW3IYHA5CNFSM4JRBCISKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEF26NHA#issuecomment-561374876>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABSXM4WCM6JWKBMT7CRG4TQW3IYHANCNFSM4JRBCISA>
.
|
|
@cquinn I looked at the commits left in this PR again and I think they are all portable and necessary (i.e. they don't hint at a problematic choice of integer types in the program). |
Nothing in the generated code will change duo this (see https://godbolt.org/z/n6Dkdp), but it makes it more clear what's happening and doesn't violate -Wsign-conversion (which I plan to enable). Without this change, clang reports: src/libponyrt/ds/list.c:52:39: error: implicit conversion changes signedness: 'unsigned long' to 'ssize_t' (aka 'long') [-Werror,-Wsign-conversion] index = ponyint_list_length(list) + index; ~ ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~ src/libponyrt/ds/list.c:52:41: error: implicit conversion changes signedness: 'ssize_t' (aka 'long') to 'unsigned long' [-Werror,-Wsign-conversion] index = ponyint_list_length(list) + index; ~ ^~~~~
The GCC/clang builtins listed here return int even though they never return a negative number. The __pony_* wrappers of these builtins all return unsigned and most of the use-cases of the builtins make sense with the unsigned result, so I think it makes sense to have unsigned return as the default for all platforms. This change means we don't have to add many explicit casts in the calls to __pony_* to fix -Wsign-conversion violations.
felipecrv
force-pushed
the
sign_conversion
branch
from
7d0245b
to
a9242fa
Compare
Enabling
-Wsign-conversion, makes the compiler report many sign conversions inlibponyrt(not a suprise). Most of them are inoffensive, but some conversions of negative integer to unsigned integers could potentially lead to infinite loops or attempts to allocatean
(unsigned)-1amount of memory.Enabling
-Wsign-conversioncan prevent these things from becoming bigger issues and hint at possible refactorings of the code (e.g. use a signed integer as the return type when a function can return negative integers).