-
Notifications
You must be signed in to change notification settings - Fork 0
[MLIR][mlir-link] Add appending linkage support #41
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
Conversation
f0be41a
to
93943cc
Compare
93943cc
to
b50e519
Compare
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.
Overall it looks good. I haven't run it on the challenge yet, but I only have minor comments that you could consider.
if (isAppendingLinkage(dstLinkage)) { | ||
auto &toAppend = append[derived.getSymbol(pair.src)]; | ||
if (toAppend.empty()) | ||
toAppend.push_back(pair.dst); | ||
if (!derived.isDeclaration(pair.src)) { | ||
toAppend.push_back(pair.src); | ||
} | ||
return ConflictResolution::LinkFromSrc; |
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.
Isn't it a bit unfortunate that we call it getConflictResolution
and then add to the list? Maybe there was no other way? Also, the name really suggests the const
to be still there...
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.
I might be able to override resolveConflict
and move the list insertion there. I will check
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.
Yep, should be fixed now
Move insertion to append list to resolveConflict
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!
No description provided.