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

Upgrade percent-encoding to 2.x and base64 to 0.12. #1349

Closed
wants to merge 1 commit into from
Closed

Upgrade percent-encoding to 2.x and base64 to 0.12. #1349

wants to merge 1 commit into from

Conversation

jakubadamw
Copy link
Contributor

No description provided.

@jakubadamw
Copy link
Contributor Author

jakubadamw commented Jun 18, 2020

I understand employing loops in the const fn context might be an overkill. 🙂 I am unsure of a better way. One way would be to convert the parser tables to use AsciiSet, as it is a rather neat structure, but there isn't even a way to create an empty AsciiSet, and so that route would require changes to percent-encoding.

@jebrosen
Copy link
Collaborator

jebrosen commented Jun 18, 2020

I understand employing loops in the const fn context might be an overkill.

Since we are working towards compiling on stable (and nearly there!), I would reject a PR that fundamentally and unconditionally requires new #![feature] flags. But, this particular PR should be possible without a feature:

One way would be to convert the parser tables to use AsciiSet, as it is a rather neat structure, but there isn't even a way to create an empty AsciiSet, and so that route would require changes to percent-encoding.

I agree using AsciiSet directly could be a cleaner approach; the one issue I know of up front is the differentiation of < and > for route specifiers only. The presets CONTROLS and NON_ALPHANUMERIC look like fine starting points - AsciiSet has both add and remove, so any set at all is possible although perhaps inconvenient. It should be possible to make a nice macro, even: ascii_set!(PCHAR_SET: CONTROLS + b'X' - b'Y');

cc #998.

@jakubadamw
Copy link
Contributor Author

jakubadamw commented Jul 1, 2020

@jebrosen, thanks for your thoughts! The const_if_match and const_loop features have just recently been stabilized in the nightly Rust. Does that address your concern?

@jebrosen
Copy link
Collaborator

jebrosen commented Jul 2, 2020

Oh, that's awkward timing - it will hit stable in 1.46.0 (currently mislabeled as 1.45.0), but I was hoping to only need to require 1.45.0 for 0.5. For that reason, I would still prefer to see a solution without the branching or loop. Since AsciiSet doesn't allow for introspection, It looks like we would have to either 1) duplicate the logic encoded in is_pchar in order to do this dependency upgrade without requiring 1.46.0, or 2) have a macro that generates both the PATH_CHARS table and the equivalent AsciiSet at the same time.

Here's the hypothetical macro I mentioned earlier and some example usage; the actual sets are untested. This could be useful as a starting point for any approach:

use percent_encoding::AsciiSet;

macro_rules! ascii_set {
    ($e:expr;) => { $e };
    ($e:expr; - $sub:literal $($rest:tt)*) => { ascii_set!($e.remove($sub); $($rest)*) };
    ($e:expr; + $add:literal $($rest:tt)*) => { ascii_set!($e.add($add); $($rest)*) };
}

const PCHAR_SET: AsciiSet = ascii_set!(
    percent_encoding::NON_ALPHANUMERIC;
    -b'!' -b'$' -b'&' -b'\''
    -b'(' -b')' -b'*' -b'+' -b',' -b'-' -b'.' -b'/'
    -b':' -b';'
    -b'=' -b'@'
    -b'_'
    -b'~'
);

const QUERY_SET: AsciiSet = ascii_set!(PCHAR_SET;
    -b'?'
    +b'%'
    +b'+'
);

@jebrosen
Copy link
Collaborator

@jakubadamw unless you have some unpushed work on this, I'm planning to rebase this onto master and finish it this week as part of some other dependency updates.

@jakubadamw
Copy link
Contributor Author

jakubadamw commented Jul 16, 2020

@jebrosen, I don't. Sure, sounds great, thanks! 🙂

@SergioBenitez SergioBenitez added the pr: closed This pull request was not merged label Jul 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: closed This pull request was not merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants