[`macro_metavar_expr_concat`] Allow `concat` in repetitions by c410-f3r · Pull Request #127720 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation22 Commits1 Checks6 Files changed

Conversation

This file contains hidden or 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

[ Show hidden characters]({{ revealButtonHref }})

r? @cjgillot

rustbot has assigned @cjgillot.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

labels

Jul 14, 2024

cjgillot added S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Jul 14, 2024

I created a new issue for the problem and referenced that in your description, your previous "fixes" would have closed the tracking issue.

Choose a reason for hiding this comment

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

rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

and removed S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Jul 15, 2024

@cjgillot If preferable, the dice of rustbot can be re-rolled

match *self {
MetaVarExpr::Count(ident, _) \| MetaVarExpr::Ignore(ident) => Some(ident),
MetaVarExpr::Concat { .. } \| MetaVarExpr::Index(..)	MetaVarExpr::Len(..) => None,
pub(crate) fn metavars(&self, mut aux: A, mut cb: impl FnMut(A, &Ident) -> A) -> A {

Choose a reason for hiding this comment

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

Could we get this signature?
This would make the logic a bit easier to read in consumer code.

pub(crate) fn metavars<A>(&self, mut aux: A, mut cb: impl FnMut(A, &Ident) -> A) -> A {
pub(crate) fn for_each_metavar(&self, mut cb: impl FnMut(&Ident)) {

Choose a reason for hiding this comment

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

That was my first attempt. Unfortunately, LockstepIterSize::with takes ownership.

The name has been changed.

dcx: DiagCtxtHandle<'a>,
ident: Ident,
interp: &FxHashMap<MacroRulesNormalizedIdent, NamedMatch>,
pnr: &ParseNtResult,
) -> PResult<'a, Symbol> {

Choose a reason for hiding this comment

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

Could you make all this a match?

Choose a reason for hiding this comment

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

Refactored to use match but it is important to note that the only diff in this function is the removal of the outer most block.

@@ -752,39 +763,37 @@ fn transcribe_metavar_expr<'a>(
}

/// Extracts an metavariable symbol that can be an identifier, a token tree or a literal.
fn extract_var_symbol<'a>(
fn extract_symbol_from_pnr<'a>(

Choose a reason for hiding this comment

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

The jargon is getting a bit obscure. It took me a while to figure what a pnr is. "Parc Naturel Régional"?

Choose a reason for hiding this comment

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

Non, ce n'est pas "Parc Naturel Régional" :)

pnr is in the function signature and extract_symbol_from_parse_nt_result would be too long IMO.

@@ -752,39 +763,37 @@ fn transcribe_metavar_expr<'a>(
}

/// Extracts an metavariable symbol that can be an identifier, a token tree or a literal.
fn extract_var_symbol<'a>(
fn extract_symbol_from_pnr<'a>(
dcx: DiagCtxtHandle<'a>,
ident: Ident,