Support List[str].index (#39210) #40348
Conversation
[ghstack-poisoned]
facebook-github-bot
added
the
oncall: jit
[ghstack-poisoned]
💊 CI failures summary and remediationsAs of commit 427d53b (more details on the Dr. CI page):
🚧 1 ongoing upstream failure:These were probably caused by upstream breakages that are not fixed yet:
ci.pytorch.org: 1 failedThis comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 13 times. |
|
|
||
| template<class T, class Iterator> | ||
| bool operator==(const ListElementReference<T, Iterator>& lhs, const T& rhs) { | ||
| T lhs_tmp = lhs; |
There was a problem hiding this comment.
this overload looks fine to me. cc @smessmer will this custom == overload have any implications on the list impl?
There was a problem hiding this comment.
Let's restrict it to only work on const ListElementReference<T, Iterator>& rhs. operator== should be symmetric or the API gets weird because a == b does something different than b == a.
There was a problem hiding this comment.
@smessmer just to confirm, is the suggestion to add a mirror version of this overload as well, or just not have overload at all?
If without this overload, then std::find() will not compile (see #40327 (comment))
There was a problem hiding this comment.
@smessmer just to confirm, is the suggestion to add a mirror version of this overload as well, or just not have overload at all?
If without this overload, then std::find() will not compile (see #40327 (comment))
@smessmer ping :)
There was a problem hiding this comment.
ok sg. I meant to remove the overload, but std::find convinced me. Having it symmetrical is fine.
wanchaol
left a comment
wanchaol
left a comment
There was a problem hiding this comment.
Looks good, could you either redirect this change to the first PR, or rename this PR with title and proper description so that we can merge it? thanks!
|
|
||
| template<class T, class Iterator> | ||
| bool operator==(const ListElementReference<T, Iterator>& lhs, const T& rhs) { | ||
| T lhs_tmp = lhs; |
There was a problem hiding this comment.
Let's restrict it to only work on const ListElementReference<T, Iterator>& rhs. operator== should be symmetric or the API gets weird because a == b does something different than b == a.
[ghstack-poisoned]
firstprayer
left a comment
firstprayer
left a comment
There was a problem hiding this comment.
added the symmetry overload
[ghstack-poisoned]
Differential Revision: [D22757035](https://our.internmc.facebook.com/intern/diff/D22757035) [ghstack-poisoned]
|
@firstprayer merged this pull request in d403983. |
Stack from ghstack:
index()on Lists. #39210)Differential Revision: D22757035