[libcxx-commits] [PATCH] D128081: [libc++] Inline the string constructors

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF added a comment.

In D128081#3597451 <https://reviews.llvm.org/D128081#3597451>, @philnik wrote:

>> Second, IMHO, the boilerplate is fine. It's not boilerplate we can get wrong, since the compiler will enforce correctness. I don' think this boilerplate is significant enough to warrant the code size changes.
>
> The compiler also knows that `std::basic_string<char, char_traits<char>, my_fancy_allocator<char>>::iterator` is correct. That doesn't mean I should use it instead of `auto`. I should, in fact, use `auto` to make it easier for myself and others to read it.

Fewer letters != more readable. `auto` contains fewer characters, but also contains zero information. The more readable code is that which spells out the return type. `std::basic_string<Char, Traits, Alloc>::iterator` is verbose, but it's also perfectly readable. 
You're in the standard library. You can read templates just fine. We optimize for correctness, not ease of readability. And while those two are related, it's important to remember which one we're actually optimizing for.

And what happens if we accidentally change the return type of the function because we changed the function body, like returning something convertible to the iterator type rather than the iterator?

Perhaps, in this case, requiring the compiler to instantiate the function in order to determine its return type isn't a problem in your example, but it is in a bunch of other cases. 
We should be using auto very sparingly. In fact, we should only ever use it when the standard mandates.

Further, I don't think moving the function bodies does anything other than make the class declaration less readable. Which is bad because ideally we should be able to visually inspect the class declaration to see if it matches the standard (std::string is a long way off from that ATM, but that's not license to make it worse).

@ldionne are you convinced this change improves readability? If so, can you say why?
@ldionne What are your thoughts on using `auto` return types in libc++ when the standard doesn't mandate it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128081