-
Notifications
You must be signed in to change notification settings - Fork 11
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
minor improvements everywhere #18
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I consider |
Closed
matklad
reviewed
Mar 15, 2020
so we currently have panic at runtime
Oh indeed, I’ve missed that....
…On Sunday, 15 March 2020, Christopher Durham ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/lib.rs
<#18 (comment)>
:
> @@ -13,3 +13,6 @@ mod traits;
mod serde_impls;
pub use crate::{range::TextRange, size::TextSize, traits::TextSized};
+
+#[cfg(target_pointer_width = "16")]
+compile_error!("text-size assumes usize >= u32 and does not work on 16-bit targets");
When the goal is to compile on multiple cfgs, I definitely agree that
specifying the behavior without cfg is better, if possible.
But for cases like this where some cfg isn't going to work at all, I don't
agree that the array shenanigans are better.
To begin with, what we have on master is "cfn_assert!
<https://crates.io/crates/const_fn_assert>" and not "const_assert!
<https://crates.io/crates/static_assertions>", so we currently have panic
at runtime (or error-by-default lint if it gets MIR const folded) behavior,
rather than guaranteed compile error.
More importantly, though, is that #[cfg] compile_error! *exists* so that
cases like this can give a useful compile error message rather than just an
opaque "const will always panic at runtime" error. And it's not like we
have to worry about other cfgs with usize < u32; Rust nominally only
supports down to a 16 bit usize (and a system with a pointer size of 256
values is constrained enough that good luck doing anything useful with
anything other than asm).
We can do both (the cfg and assert), if you prefer, but the cfg pays for
itself by providing a useful compile error and documentation of intent in
this case.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#18 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AANB3M7VB6VEN4GL2ABHZE3RHTLQRANCNFSM4LIBIDBQ>
.
|
matklad
reviewed
Mar 19, 2020
matklad
reviewed
Mar 19, 2020
it would have made sense to send |
Yeah I know I just sort of ended up lumping everything in this PR by accident. |
I think I've pruned this back to the approved changes now. |
Closed
bors r+ |
Build succeeded |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Because none of this was quite specific enough to break into its own PR
compiler_error!
-forbidcfg(target_pointer_width = "16")
TextRange::before
(alt options:TextRange::to
,TextRange::up_to
,TextRangeTo
,TextSize::prefix
)TextRange::after
(alt options:TextSize::suffix
)TextRange::offset
(alt options:TextRange::add
,Add::add
,TextRange::reanchor
)TextSize::one
(alt options: )TextSize::ONE
(alt options:TextSize::ASCII
#[inline]
capability for a bunch of thingsThis PR is mainly just to get opinions on all of these little things, then I'll re-force-push with a commit just with those.