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

xkbcomp/expr: remove unused ExprResolveKeyCode #654

Merged
merged 2 commits into from
Feb 7, 2025

Conversation

bluetech
Copy link
Member

@bluetech bluetech commented Feb 7, 2025

This function was added in commit 4e22851. But that commit also changed the grammar:

-KeyNameDecl    :       KeyName EQUALS Expr SEMI
+KeyNameDecl    :       KeyName EQUALS KeyCode SEMI

i.e. while before you could write

<AE01> = 9+1;

now this is a syntax error, an integer literal is expected. I'm not sure if it was intended to remove this ability. In any case, this rendered ExprResolveKeyCode useless since there's no longer an expression to evaluate, and after some refactoring it went unused.

Even if we choose to restore Expr here, I don't see a reason for the specialized function over ExprResolveInteger except the type (which we should probably widen from int to int64_t...). So remove it.

  • Commit which removes an outdated comment.

This function was added in commit
4e22851. But that commit also changed
the grammar:

-KeyNameDecl    :       KeyName EQUALS Expr SEMI
+KeyNameDecl    :       KeyName EQUALS KeyCode SEMI

i.e. while before you could write

    <AE01> = 9+1;

now this is a syntax error, an integer literal is expected. I'm not sure
if it was intended to remove this ability. In any case, this rendered
`ExprResolveKeyCode` useless since there's no longer an expression to
evaluate, and after some refactoring it went unused.

Even if we choose to restore Expr here, I don't see a reason for the
specialized function over `ExprResolveInteger` except the type (which we
should probably widen from int to int64_t...). So remove it.

Signed-off-by: Ran Benita <[email protected]>
What it says mostly no longer holds, I think it's more confusing than
helpful now.

Signed-off-by: Ran Benita <[email protected]>
@wismill
Copy link
Member

wismill commented Feb 7, 2025

Agreed, I did this too in #605.

which we should probably widen from int to int64_t...

See #605

@bluetech
Copy link
Member Author

bluetech commented Feb 7, 2025

Ah missed it sorry! The first two commits from #605 look good to me, if you'd like to cherry pick them.

@bluetech bluetech merged commit bb5e464 into xkbcommon:master Feb 7, 2025
5 checks passed
@bluetech bluetech deleted the rm-resolve-keycode branch February 7, 2025 18:55
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