Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove cast warnings #186

Merged
merged 1 commit into from
May 23, 2023
Merged

remove cast warnings #186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions stew/base58.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,13 +170,13 @@ proc decode*[T: byte|char](btype: typedesc[Base58C], instr: openArray[T],
result = Base58Status.Incorrect
return
let ch = alphabet.decode[int8(instr[i])]
if ch == -1:
if ch < 0:
outlen = 0
result = Base58Status.Incorrect
return
var c = cast[uint32](ch)
var c = uint32(ch)
for j in countdown(size - 1, 0):
let t = cast[uint64](buffer[j]) * 58 + c
let t = uint64(buffer[j]) * 58 + c
c = cast[uint32]((t and 0x3F_0000_0000'u64) shr 32)
buffer[j] = cast[uint32](t and 0xFFFF_FFFF'u32)
if c != 0:
Expand Down
6 changes: 3 additions & 3 deletions stew/bitops2.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ func log2truncNim(x: uint8|uint16|uint32): int =
v = v or v shr 4
v = v or v shr 8
v = v or v shr 16
cast[int](lookup[uint32(v * 0x07C4ACDD'u32) shr 27])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the cast here is intentional to avoid a runtime check of the returned value - so this should be cast[int](uint(lookup... - ditto other int casts below

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the compiler smart enough to not put checks that are impossible? uint8->int is always safe and doesn't require checks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

var xxx = 5'u8
var yyy: int = int(xxx)
echo yyy

=>

N_LIB_PRIVATE NU8 xxx__test_1 = ((NU8)5);
N_LIB_PRIVATE NI yyy__test_2;
....
        nimln_(2, "/tmp/test.nim");
        yyy__test_2 = ((NI) (xxx__test_1));
        nimln_(3, "/tmp/test.nim");

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah good point, if this indeed works - it should be able to do it, but that's one side of the coin only ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if it doesn't, and there's a reproducible test case, that sounds like a good candidate for a Nim bug to file. In the meantime, the correctness per se of removing casts wouldn't be in question, just efficiency, in versions of Nim which don't receive said backports.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My motivation was this comment nim-lang/Nim#20103 (comment).

Is type cast for different number of bits well-defined in Nim?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about const lookup: array[32, int]?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

safeConvertint

#34 - this would / should be covered by the "lossless conversion" part of that framework - the idea generalises quite well.

I'm somewhat against putting "safe" in names frivolously without defining "safe" generally in the language - it gets overloaded for a bunch of confusing and sometimes contradictory purposes since "safe" on its own has no real meaning.

Since the language already warns against "unsafe" usage of conversions specifically, all we need to do is to turn the warning into an error once all known uses are fixed.

Is type cast for different number of bits well-defined in Nim?

no - that's why there's a warning for it now - casting to a larger type is UB (in other words: if ever you manage to cast to a larger type without a warning being printed, it's a bug that should be reported upstream).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the language already warns against "unsafe" usage of conversions specifically, all we need to do is to turn the warning into an error once all known uses are fixed.

Oops, right, that was silly: it doesn't warn in all cases, specifically the ones we're discussing here :)

Ok, so I agree a construct would be useful that would allow the defect-free conversions.

Copy link
Contributor

@Menduist Menduist May 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would / should be covered by the "lossless conversion" part of that framework

👍, would definitively be nice to have this framework set up, could also include the work that was done on enum conversion #115

int(lookup[uint32(v * 0x07C4ACDD'u32) shr 27])

func log2truncNim(x: uint64): int =
## Quickly find the log base 2 of a 64-bit integer.
Expand Down Expand Up @@ -169,9 +169,9 @@ when (defined(gcc) or defined(llvm_gcc) or defined(clang)) and useBuiltins:
cast[int](builtin_ffs(cast[cint](x.cuint)))

func log2truncBuiltin(v: uint8|uint16|uint32): int =
cast[int](31 - cast[cuint](builtin_clz(v.uint32)))
int(31 - cast[cuint](builtin_clz(v.uint32)))
func log2truncBuiltin(v: uint64): int =
cast[int](63 - cast[cuint](builtin_clzll(v)))
int(63 - cast[cuint](builtin_clzll(v)))

elif defined(vcc) and useBuiltins:
const arch64 = sizeof(int) == 8
Expand Down