[libcxx-commits] [PATCH] D131668: [libc++] Implement P2438R2 (std::string::substr() &&)

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/string:892
+
+        _Traits::move(data(), data() + __pos, __len);
+        __set_size(__len);
----------------
var-const wrote:
> var-const wrote:
> > philnik wrote:
> > > philnik wrote:
> > > > var-const wrote:
> > > > > var-const wrote:
> > > > > > Consider a comment like `Move the given string then shrink it in place`.
> > > > > Reevaluating `data()` calls `to_address()` and `is_long()` repeatedly. Would it make sense to store the result in an intermediate variable?
> > > > If the compiler can't optimize it IMO we should give the compiler hints on `data()` instead of decreasing the readability of the code.
> > > I think this comment is more misleading than helpful. `_Traits::move()` and `__set_size()` seem to me quite intuitive. `shrink in place` sounds a lot like we `shrink_to_fit()` here, which we definitely don't want to do.
> > Personally, I find using a variable more readable (for me, the repeated function calls are "noisy" and add extra cognitive overhead of thinking whether the function result might change upon subsequent calls). Also, performance in debug mode is a common concern among users.
> Ping.
I don't think we can do much about debug performance in the library. Saving the result won't make much of a difference compared to letting the compiler optimize a bit. Even calling an empty function has quite a bit of overhead that isn't observable in any way AFAIK.
```
void func() {}
```
produces
```
        push    rbp
        mov     rbp, rsp
        pop     rbp
        ret
```
but it could just be `ret` without any problems. That's not something we can influence in the library.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131668/new/

https://reviews.llvm.org/D131668