fix: AppleClang 12 warnings#2510
Conversation
|
|
||
| void process(list &args_list, detail::args_proxy ap) { | ||
| for (const auto &a : ap) | ||
| for (auto a : ap) |
There was a problem hiding this comment.
In case anyone else wonders "why don't we fix the iterator class", here's why:
- "Iterator" is this thing
operator*returns apy::handlewhich is generally better to copy than to take by reference.- The iterator stores and mutates
value, so returning a reference here would be wrong as soon as you advance the iterator.
So this is complete fine.
There was a problem hiding this comment.
Meep! I guess I haven't looked what iterator / const_iterator look like for non-random-access containers in STL 😅
From my understanding, your statement of "why don't we fix the iterator class" means "why don't we change operator* return [const] handle&", which is 100% valid in terms of this PR.
I think there's still question like "what do other libs do for similar patterns and should we adopt that pattern if it enables const T&?", especially for iterators implemented for something like std::list, etc.
There was a problem hiding this comment.
(No action here, to be clear, just wanted to clarify on the comment)
EricCousineau-TRI
left a comment
EricCousineau-TRI
left a comment
There was a problem hiding this comment.
Thanks for the heads up on the Apple #ifdef change!
|
Thanks for the extra clarification, @bstaletic & @EricCousineau-TRI! Given that this is a simple fix, I think it can safely go in. |
Fixed in pybind#2510 but reintroduced on one line by pybind#2126
4 participants
Fixing two sets of warnings in AppleClang 12. First is a warning that you can't really take a reference to a temporary object, so this will actually always be a copy, and should be clearly written as such.
Second is a bug where 11.0.0 or 12.0.0 or X.X.0 AppleClang will produce a warning due not passing an if statement. It's just a pragma so it should be safe even if the version doesn't match? CC @EricCousineau-TRI (I think).
Closes #2509