Untag dispatch tree #2287
Untag dispatch tree #2287
Conversation
Towards microsoft#189
| } else { | ||
| if constexpr (_Is_set) { | ||
| return _Buynode(_STD move(_Val)); | ||
| } else { | ||
| return _Buynode(_STD move(const_cast<key_type&>(_Val.first)), _STD move(_Val.second)); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think you can flatten that into one else if
| } else { | |
| if constexpr (_Is_set) { | |
| return _Buynode(_STD move(_Val)); | |
| } else { | |
| return _Buynode(_STD move(const_cast<key_type&>(_Val.first)), _STD move(_Val.second)); | |
| } | |
| } | |
| } else if constexpr (_Is_set) { | |
| return _Buynode(_STD move(_Val)); | |
| } else { | |
| return _Buynode(_STD move(const_cast<key_type&>(_Val.first)), _STD move(_Val.second)); | |
| } |
There was a problem hiding this comment.
I think unflatten is more clear
There was a problem hiding this comment.
Yeah not a hill I would die on, feel free to disregard
There was a problem hiding this comment.
I'm not sure. I'll wait for other reviewers opinion.
There was a problem hiding this comment.
They're pretty close, but I agree with Alex that the unflattened structure is clearer. The rationale is that this is a forked path, with a sub-fork, which is shown by the unflattened structure. It would be equally clear (ignoring our "prefer positive tests" convention) if the branches were reversed. Flattening the logic would make the order of the tests important, and having to worry about the order in which tests are performed is a slight mental burden.
There was a problem hiding this comment.
I also have a mild preference to unflatten, so it's a tie, and the status quo code in the PR stays.
| } else { | ||
| if constexpr (_Is_set) { | ||
| return _Buynode(_STD move(_Val)); | ||
| } else { | ||
| return _Buynode(_STD move(const_cast<key_type&>(_Val.first)), _STD move(_Val.second)); | ||
| } | ||
| } |
There was a problem hiding this comment.
I also have a mild preference to unflatten, so it's a tie, and the status quo code in the PR stays.
|
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
|
Thanks for improving the ordered associative containers! 🌲 🌳 🌴 |
Towards #189