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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

In D128081#3600370 <https://reviews.llvm.org/D128081#3600370>, @EricWF wrote:

> @ldionne are you convinced this change improves readability? If so, can you say why?

Yes, I personally do find that it does improve readability. I've always been put off by the fact that we define functions out-of-line, and have to repeat all kinds of crazy declarations (and often complicated SFINAE). To me that just doesn't make much sense, given that our API is dictated by the Standard and documented in our top-of-header synopsis. So repeating the declarations one more time in the class declaration does not add a lot of value, in my opinion. Of course in the end this is kind of a matter of taste.

> @ldionne What are your thoughts on using `auto` return types in libc++ when the standard doesn't mandate it?

I don't think that would be a good idea, but this patch doesn't seem to introduce this so I think we're good.

--------------------

I think the real potential concern with this patch is indeed turning functions that were not marked as `inline` into `inline` functions, since that could have an impact on codegen. I suggest we make this patch a NFC by only moving functions that were already marked as `inline` into the class. After that, we should be left with only out-of-line functions outside of the class. We won't know which ones were intentionally left out-of-line and which ones were accidental, but we can take a stab and consider each function individually, moving them into the class when we think it's the intent. That can be done in followups.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128081