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

Fixed uuid!("") macro diagnostics when using urn:uuid prefix. #554

Merged
merged 2 commits into from Nov 6, 2021

Conversation

QnnOkabayashi
Copy link
Contributor

I'm submitting a bug fix

Description

Fixed an bug where the uuid!("") macro would point to the wrong characters when using the urn:uuid: prefix or wrapping with the curly braces ("{...}").

Related Issue(s)

none

@KodrAus
Copy link
Member

KodrAus commented Nov 2, 2021

Nice catch!

Looks like we’ve got a couple test failures here. I think the start variable is expected to be fixed, so we might need to track the index to report errors at from a different starting point?

We also just need to make sure the parser will never panic. Maybe that should go in a comment at the top of the function?

@QnnOkabayashi
Copy link
Contributor Author

QnnOkabayashi commented Nov 2, 2021

Nice catch!

Looks like we’ve got a couple test failures here. I think the start variable is expected to be fixed, so we might need to track the index to report errors at from a different starting point?

This was caused by a usize subtraction overflow, which is now fixed.

We also just need to make sure the parser will never panic. Maybe that should go in a comment at the top of the function?

I'm fairly confident it won't panic anymore. I also found out that it panicked when a wide char was present, so I had to add some support in the macro that fixed this. For example, the macro no longer panics on multibyte unicode:

error: invalid character: expected an optional prefix of `urn:uuid:` followed by 0123456789abcdefABCDEF-, found 岡 at 6
  --> tests/ui/compile_fail/invalid_parse.rs:37:30
   |
37 | const _: Uuid = uuid!("504410岡林aab1426f9247bb680e5fe0c8");
   |                              ^^

error: invalid character: expected an optional prefix of `urn:uuid:` followed by 0123456789abcdefABCDEF-, found 😎 at 6
  --> tests/ui/compile_fail/invalid_parse.rs:38:30
   |
38 | const _: Uuid = uuid!("504410😎👍aab1426f9247bb680e5fe0c8");
   |                              ^^

Also, I added some util functions for creating Errors to clean up the parsing algorithm a bit.

}
.into());
// let found = input[i_char..].chars().next().unwrap();
// let found = char::from(chr);
Copy link
Member

Choose a reason for hiding this comment

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

Did we mean to keep these comments here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope that was an accident

const _: Uuid = uuid!("67e550-4105b1426f9247bb680e5fe0c");

const _: Uuid = uuid!("504410岡林aab1426f9247bb680e5fe0c8");
const _: Uuid = uuid!("504410😎👍aab1426f9247bb680e5fe0c8");
Copy link
Member

Choose a reason for hiding this comment

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

😎 👍

@KodrAus
Copy link
Member

KodrAus commented Nov 2, 2021

That is much easier to follow. Do we have any test cases for the parse_str function itself that includes wide characters?

@QnnOkabayashi
Copy link
Contributor Author

That is much easier to follow. Do we have any test cases for the parse_str function itself that includes wide characters?

Not yet. Also how optimized is the parsing algorithm? The more I look at it, the more it bothers me how clunky it feels, and I'd like to refactor it if you're not opposed.

@KodrAus
Copy link
Member

KodrAus commented Nov 3, 2021

I'd like to refactor it if you're not opposed.

I think it's had one or two rounds of optimization applied in its lifetime, but nothing like a complete rewrite as far as I remember. If you'd like to spend some time on it that sounds fantastic! I don't mind if you just want to keep pushing changes to this PR here, or if we merge and start another.

@KodrAus KodrAus merged commit fa28942 into uuid-rs:main Nov 6, 2021
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.

None yet

2 participants