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

feat: Add a TryFrom<&str> implementation for enumerations #224

Merged
merged 2 commits into from
Feb 7, 2022

Conversation

Marwes
Copy link
Member

@Marwes Marwes commented Nov 21, 2021

Closes #223

cc @krobelus

Copy link
Contributor

@krobelus krobelus left a comment

Choose a reason for hiding this comment

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

yep that works for me. I still haven't gotten to the botom of why not all case conversions are exercised at compile time, but that's not blocking.

@@ -43,7 +68,7 @@ fn fmt_pascal_case(f: &mut std::fmt::Formatter<'_>, name: &str) -> std::fmt::Res
}

macro_rules! lsp_enum {
(impl $typ: ty { $( $(#[$attr:meta])* pub const $name: ident : $enum_type: ty = $value: expr; )* }) => {
(impl $typ: ident { $( $(#[$attr:meta])* pub const $name: ident : $enum_type: ty = $value: expr; )* }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure why, also the other : ty is still here

src/lib.rs Outdated
impl std::convert::TryFrom<&str> for $typ {
type Error = &'static str;
fn try_from(value: &str) -> Result<Self, Self::Error> {
match value {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can also write match ()

src/lib.rs Outdated
fn try_from(value: &str) -> Result<Self, Self::Error> {
match value {
$(
_ if { let (buf, len) = crate::fmt_pascal_case_const(stringify!($name)); &buf[..len] == value.as_bytes() } => Ok(Self::$name),
Copy link
Contributor

Choose a reason for hiding this comment

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

great idea. The only weird thing is that when I change the max length from 128 to 5 we will error at runtime (not at compile time!) if the value parameter is only known at runtime -- even though $name is known at compile time.
Assertions probably won't help here either (though they should be allowed in stable Rust soon)

So we might save allocations but we still run the code at runtime and don't generate the neat match-statement a human would write, which is "TypeParameter" => Ok(Self::TYPE_PARAMETER)

Anyway, that's not so important.

Copy link
Contributor

Choose a reason for hiding this comment

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

So at this point it seems better to write
match pascal_case_to_screaming_snake_case(value) { "TEXT" => Ok(Self::Text), .. }

_ if {
const X: (crate::PascalCaseBuf, usize) = crate::fmt_pascal_case_const(stringify!($name));
let (buf, len) = X;
&buf[..len] == value.as_bytes()
Copy link
Contributor

Choose a reason for hiding this comment

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

pretty nice - this commit reduces the instruction count of the CompletionItemKind::try_from() from 4000 to just above 300 (in release mode, debug has around 10x more). Now let's hope that future rust will do this without the explicit const :)

krobelus added a commit to krobelus/kakoune-lsp that referenced this pull request Nov 28, 2021
The new next/prev symbol feature is typically used to jump around
functions. We even provide a convenience wrapper with a default
mapping. Some programming styles use mostly methods instead of
functions. To make the default mapping more useful, extend it to
jump to functions *or* methods, instead of just methods.  Delete the
convenience wrapper for methods, since that's now superseded.

Swap the argument order of lsp-next-symbol and friends, so we
can take a variable number of symbol kinds.

Add a deserialization method until this feature becomes part of
lsp_types, see gluon-lang/lsp-types#224
krobelus added a commit to kakoune-lsp/kakoune-lsp that referenced this pull request Nov 28, 2021
The new next/prev symbol feature is typically used to jump around
functions. We even provide a convenience wrapper with a default
mapping. Some programming styles use mostly methods instead of
functions. To make the default mapping more useful, extend it to
jump to functions *or* methods, instead of just methods.  Delete the
convenience wrapper for methods, since that's now superseded.

Swap the argument order of lsp-next-symbol and friends, so we
can take a variable number of symbol kinds.

Add a deserialization method until this feature becomes part of
lsp_types, see gluon-lang/lsp-types#224
topisani pushed a commit to kakoune-lsp/kakoune-lsp that referenced this pull request Dec 15, 2021
The new next/prev symbol feature is typically used to jump around
functions. We even provide a convenience wrapper with a default
mapping. Some programming styles use mostly methods instead of
functions. To make the default mapping more useful, extend it to
jump to functions *or* methods, instead of just methods.  Delete the
convenience wrapper for methods, since that's now superseded.

Swap the argument order of lsp-next-symbol and friends, so we
can take a variable number of symbol kinds.

Add a deserialization method until this feature becomes part of
lsp_types, see gluon-lang/lsp-types#224
@Marwes Marwes merged commit d2d3c37 into master Feb 7, 2022
@Marwes Marwes deleted the try_from_string branch February 7, 2022 11:41
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.

2 participants