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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

> First, this has a lot of functional changes.

Have I ever claimed there aren't any functional changes?

> Like allowing the compiler to inline these functions, which we've explicitly externally instantiated and were consciously put out of line for that reason.

I'm aware that there are some functions we've consciously not marked `inline`, but I don't know which ones they are (assuming there are any which aren't explicitly declared in `extern_template_lists.h`). For example `basic_string(const CharT*, const Allocator&)` isn't marked inline, but also isn't in the exported functions list. It looks to me a lot like we've just forgotten to mark it `inline`. If we do that consciously I'd argue that we should write it inline and mark it `_LIBCPP_NOINLINE` or something like it. That makes it obvious that we didn't just forget to mark a function `inline`.

> This will result in a large code size increase.
> And it's not something I think we should do to our users. It largely negates the ABI exports libc++ intentional created.

Most of the functions I've inlined were already marked `inline`, so for those this is purely NFC.

> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128081