-
Notifications
You must be signed in to change notification settings - Fork 281
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
[FIRRTL] Clean up RWProbe's verifier #5806
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Testing error paths is good, would've revealed this. Thanks for fixing + testing!
if (targetRef.getModule() != | ||
(*this)->getParentOfType<FModuleLike>().getModuleNameAttr()) | ||
return emitOpError() << "has non-local target"; | ||
|
||
auto target = ns.lookup(targetRef); | ||
if (!target) | ||
return emitOpError() << "has target that cannot be resolved: " << target; | ||
return emitOpError() << "has target that cannot be resolved: " << targetRef; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oof, thanks!
if (!targetRef) | ||
return emitOpError("has invalid target reference"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Thanks for catching this.
Two changes: 1) The ODS generated verifier checks that the operation has a valid `targetRef` attribute, so there is no need to check it ourselves in the verifier. This is currently an untestable code-path. 2) When the target is invalid, it prints as `<invalid target>`. It makes more sense to print the original InnerRefAttr for this case. This change adds a test case for this.
d69de22
to
8ee2d58
Compare
Two changes:
targetRef
attribute, so there is no need to check it ourselves in the verifier. This is currently an untestable code-path.<invalid target>
. It makes more sense to print the original InnerRefAttr for this case. This change adds a test case for this.