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.

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

merged 1 commit into from
May 23, 2023

Conversation

diegomrsantos
Copy link
Contributor

No description provided.

@@ -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

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

@diegomrsantos diegomrsantos requested a review from arnetheduck May 18, 2023 18:51
@diegomrsantos diegomrsantos merged commit 003fe9f into master May 23, 2023
@diegomrsantos diegomrsantos deleted the remove-warnings branch May 23, 2023 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants