[libcxx-commits] [libcxx] [libc++][NFC] Simplify copy and move lowering to memmove a bit (PR #83574)

Konstantin Varlamov via libcxx-commits libcxx-commits at lists.llvm.org
Wed Mar 20 00:09:13 PDT 2024


https://github.com/var-const requested changes to this pull request.

First, thanks a lot for this refactoring! It removes quite a bit of boilerplate. (And I really hope that we can simplify further using `if constexpr` in the midterm future)

Let me make sure I understand the refactoring correctly. All the logic for overload resolution is now in the algorithm-specific functor (e.g., inside `__copy_loop`), therefore `copy_move_common.h` no longer needs to deal with selecting the right algorithm, which ends up being less code. Is that right? If that's the case, we need to update some of the names -- IMO, they made sense in the earlier state but became a little stale with this refactoring. I initially found the diff pretty confusing, and I think the leftover names are a big reason for that, they're almost misleading at this point:
- `__dispatch_copy_or_move` -- this function used to be the central point dispatching to the right algorithm (whether it's the "naive" for-loop implementation or the optimized implementation). It no longer really dispatches much. I'd suggest something like `__run_with_optional_unwrapping`, `__maybe_unwrap_and_run`, etc.;
- names like `__copy_loop` and `__copy_trivial` used to complement each other, describing two alternative implementations of a given algorithm. Now that `__copy_trivial` is no longer a thing, `__copy_loop` doesn't really make sense, especially considering that the functor now contains both the loop and non-loop implementations. I'd be fine with something very generic like `__copy_func`, `__copy_impl`, etc., but IMO we should drop the `_loop` part, it's redundant at best and I'd say even misleading at this point.

With that, we would have something like `__copy_func` that contains 3 mutually-SFINAE'd implementation alternatives and is invoked via a `__maybe_unwrap_iters_and_run` helper, which IMO is easier to follow. Of course, all of these names can be further improved, the important part is changing the stale names.

Other than the names, pretty much LGTM.

https://github.com/llvm/llvm-project/pull/83574


More information about the libcxx-commits mailing list