mbe: Rework the concat metavariable expression
#142975
Draft
+1,584
−1,154
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
These tests have expanded beyond the RFC, so rename the directory `rfc-3086-metavar-expr` to `metavar-expressions`. `concat` (which wasn't part of the RFC) now fits in this group, so merge its tests into the `metavar-expressions` directory. Additionally rename some related `issue-*` tests.
`syntax-errors` currently contains both actual syntax error tests and tests (which don't need expansion) and tests for incorrect usage of the `count` metavariable expression (which dos need expansion). Split the expansion-dependent tests to a separate file, remove unneeded invocations from `syntax-errors` to ensure we are catching these before expansion.
Add tests showing the current state to make it more clear when output gets updated later in refactoring.
More diagnostic structs related to metavariable expressions will be introduced. Introduce the abbreviation "mve" which is reasonably unambiguous (`rg Mve` and `rg '(\b|_|-)mve(\b|_|-)'` return no results outside of a Thumb target feature) and use it for these diagnostic types. A new module is also created.
Move the `concat` implementation to a separate function so it is easier to work on. Other metavariable expressions are already split this way. This is a non-functional change.
Move this structure directly above the `parse_<expr>` functions that return it to keep top-down flow. This is a non-functional change.
The current messages have the potential to be more accurate; "expected identifier or string literal" is printed in a few cases where only an identifier should be expected, and it suggests removing string literals when that might not solve the problem. Add a new diagnostic for these kind of errors that gives some more context. For `count` and `ignore` this should likely be combined with the diagnositcs for `eat_dollar` to produce a helpful error if they get anything other than a metavariable first argument. I am planning to do this in a followup.
Give a more user-friendly diagnostic about the following:
* Invalid syntax within the `${...}` braces, including missing
parentheses or trailing tokens.
* Incorrect number of arguments passed to specific metavariable
expressions.
* Now accept numbers, chars * Suffixes are stripped (needs more testing) * Report specific locations of errors * TODO: handle idents the same for expanded tokens
rustbot
added
A-tidy
|
The job Click to see the possible cause of the failure (guessed by this bot) |
tgross35
added
the
rla-silenced
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.
#142950 plus followup
r? @ghost