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.

Sign up for GitHub

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

Jump to bottom

OpAsmParser::OperandType name is really confusing #54446

Closed
silvasean opened this issue Mar 18, 2022 · 4 comments
Closed

OpAsmParser::OperandType name is really confusing #54446

silvasean opened this issue Mar 18, 2022 · 4 comments
Assignees
@zero9178
Labels
mlir:core MLIR Core Infrastructure

Comments

@silvasean
Copy link
Contributor

Every time I use this type I get confused by the fact that it has "type" in the name. Even the comment above it says:

  /// This is the representation of an operand reference.
  struct OperandType {

So wouldn't OperandReference be a better name?
@silvasean silvasean added the mlir:core MLIR Core Infrastructure label Mar 18, 2022
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 18, 2022

@llvm/issue-subscribers-mlir-core

@lattner
Copy link
Collaborator

lattner commented Mar 18, 2022

Yes, completely agreed. OperandUtterance is even more specific (but no, we shouldn't use that :) because it really is about a lexical occurrence of an operand reference like %y or %x#4.

@zero9178 zero9178 self-assigned this Mar 21, 2022
@zero9178
Copy link
Member

Patch candidate: https://reviews.llvm.org/D122142

@zero9178 zero9178 added the awaiting-review Has pending Phabricator review label Mar 21, 2022
@github-actions github-actions bot removed the awaiting-review Has pending Phabricator review label Mar 21, 2022
@silvasean
Copy link
Contributor Author

Thanks for the quick turnaround time on this @zero9178 !! Really impressive :)

mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this issue Oct 7, 2022
@zero9178
[mlir] Rename OpAsmParser::OperandType to `OpAsmParser::UnresolvedO…
c56a5b5 
…perand`

I am not sure about the meaning of Type in the name (was it meant be interpreted as Kind?), and given the importance and meaning of Type in the context of MLIR, its probably better to rename it. Given the comment in the source code, the suggestion in the GitHub issue and the final discussions in the review, this patch renames the OperandType to UnresolvedOperand.

Fixes llvm/llvm-project#54446

Differential Revision: https://reviews.llvm.org/D122142
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@zero9178 zero9178

Labels
mlir:core MLIR Core Infrastructure
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@zero9178 @lattner @silvasean @llvmbot