Details
- Reviewers
ldionne • Quuxplusone Mordante - Group Reviewers
Restricted Project
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
Thanks for working on these cleanups! In general LGTM when the CI passes.
libcxx/include/future | ||
---|---|---|
1636 | As a note for other followup patches. I prefer these clang-tidy improvements not to do other cleanups in the same patch. I would prefer to see that in different patches. This makes reviewing them easier. |
libcxx/include/__functional/function.h | ||
---|---|---|
267 | FWIW, my personal style on the OOP polymorphic types like __base (where they are clearly just abstract base classes with lots of pure virtual methods) is not to bother with the special member functions at all. Nobody's seriously going to be value-semantic-copying this type, so why waste the two LoC preventing it? | |
libcxx/include/__functional/reference_wrapper.h | ||
37–38 | Eliminate the NOLINT comment and just make these two declarations public; it can't hurt. Ditto in ref_view below. | |
libcxx/include/iosfwd | ||
262 | Stylistically, I'd prefer to see these placed between new lines 278 and 279, so it goes "ctors, assignment operators, dtors." Ditto throughout (at least in the cases where you're already shuffling code around). | |
libcxx/include/tuple | ||
438–441 | Consider west-consting line 363 at the same time, and also doing something explicitly about __tuple_leaf& operator=(__tuple_leaf&&). I actually don't know off the top of my head what happens to it right now. |
Are we sure this doesn't break the ABI because changing user declared constructors into delete constructors can do that, Can't it?
How could it change the ABI? The functions can never be called, and it doesn't change any type traits AFAIK. It's not like with = default that the type could be trivially_something after the change that it wasn't before.
From the Itanium C++ ABI Specification:
non-trivial for the purposes of calls
A type is considered non-trivial for the purposes of calls if:
- it has a non-trivial copy constructor, move constructor, or destructor, or
- all of its copy and move constructors are deleted.
This definition, as applied to class types, is intended to be the complement of the definition in [class.temporary]p3 of types for which an extra temporary is allowed when passing or returning a type. A type which is trivial for the purposes of the ABI will be passed and returned according to the rules of the base C ABI, e.g. in registers; often this has the effect of performing a > trivial copy of the type.
@EricWF And in what scenario would that change the ABI? If there are no copy- or move-constructors, then all are deleted by definition, and if there is one it isn't trivial.
@philnik: Eric is at least partly correct (and your original goalpost was wrong): this transformation can definitely change the is_trivially_fooable properties of the type. However, I haven't yet been able to get this to cause an actual calling-convention difference. https://godbolt.org/z/d17PM8dWc It seems to me that the wording Eric quoted was probably designed precisely to prevent the breakage Eric is concerned about! It basically says, "Deleted SMFs don't affect triviality, unless you're using deleted SMFs to replicate the old C++98 trick of declare-but-not-defining all your SMFs. If all your SMFs are deleted, then we'll assume you still want to be non-trivial for the purposes of calls."
So I believe Eric is correct about the possibility of breakage here, and I think you should go look at the triviality-for-purposes-of-calls of all of the affected classes (i.e., don't just trust clang-tidy's mechanical change; go through it line by line). I predict you'll find that every affected class is either still non-trivial for other reasons (e.g. virtual dtor), or is a detail type like __save_flags where we don't care about its triviality. But yeah, Eric's convinced me that we shouldn't just assume this is safe.
The important bit is this (emphasis mine):
A type is considered non-trivial for the purposes of calls if:
- it has a non-trivial copy constructor, move constructor, or destructor, or
- all of its copy and move constructors are deleted.
I went through all the changes in this patch, and I think the key question we need an answer to is:
When a copy assignment operator or copy constructor is explicitly deleted, there is no implicit move constructor generated by the compiler. But does the compiler not declare any move constructor, or does it declare one and mark it as deleted? I wasn't able to find out by reading http://eel.is/c++draft/class.copy.ctor, but it may be easy to answer for others here.
This is relevant because some classes go from having a user-declared private operator=(T const&) to having a deleted one. I believe it's possible for such a type to be trivial for the purpose of calls. However, if we change it to operator(T const&) = delete and the compiler then implicitly declares T(T const&) = delete, we'd go from trivial to non-trivial.
Anyway, I've left some not-done comments on the ones where I'm not 100% confident that this patch isn't changing behavior. I suspect it is correct, I just need to understand why.
Note: In my review, TFPC stands for "Trivial For the Purpose of Call"
libcxx/include/__bit_reference | ||
---|---|---|
168 | I'm not sure about this one, see question. | |
libcxx/include/__functional/function.h | ||
265 | Code was not TFPC before, and it's not after because there's a user-defined destructor. | |
libcxx/include/__locale | ||
60 | Code was not TFPC before, and it's not after because there's a user-defined destructor. | |
libcxx/include/__split_buffer | ||
76 ↗ | (On Diff #413792) | Code was not TFPC, and isn't after patch because we have a move ctor but it is user-defined. |
libcxx/include/deque | ||
1042 ↗ | (On Diff #413792) | Code was not TFPC before, and it's not after because there's a user-defined destructor. |
libcxx/include/forward_list | ||
536–537 | Code was not TFPC before, and it's not after because there's a user-defined destructor. | |
libcxx/include/future | ||
1634 | Code was not TFPC before, and it's not after because there's a user-defined destructor. | |
libcxx/include/iosfwd | ||
261 | Code was not TFPC before, and it's not after because there's a user-defined destructor. | |
libcxx/include/list | ||
552 | Can we move these down near the other constructors? | |
554 | Code was not TFPC before, and it's not after because there's a user-defined destructor. | |
libcxx/include/locale | ||
3641 | Code was not TFPC before, and it's not after because there's a user-defined destructor. | |
3917 | Code was not TFPC before, and it's not after because there's a user-defined destructor. | |
libcxx/include/map | ||
718 | I'm not sure about this one. I think it was TFPC before the patch, because there were no copy/move ctors declared. After the patch, there shouldn't be any copy/move ctor declared unless the compiler declares them as deleted, in which case it would become non-TFPC. | |
libcxx/include/regex | ||
1453 | Code was not TFPC before, and it's not after because there's a user-defined destructor. | |
2227 | I'm not sure about this one, see question. | |
2268 | I'm not sure about this one, see question. | |
2310 | I'm not sure about this one, see question. | |
2366 | I'm not sure about this one, see question. | |
3159 | I'm not sure about this one, see question. | |
libcxx/include/thread | ||
164 | Code was not TFPC before, and it's not after because there's a user-defined destructor. | |
libcxx/include/tuple | ||
370 | This one has defaulted copy and move constructors, so this won't change triviality. | |
438 | This one has defaulted copy and move constructors, so this won't change triviality. | |
libcxx/include/unordered_map | ||
796 | This one is not TFPC before and not TFPC after because it has user-defined copy/move ctors. | |
libcxx/include/vector | ||
802 | Code was not TFPC before, and it's not after because there's a user-defined destructor. Also this type should not be ABI affecting, but w/e. |
I think the compiler doesn't declare one. The explicitly deleted copy constructor counts as a user-declated constructor, which means that no move constructor will be declared. Deleting a copy assignment operator also doesn't delete the copy constructor (https://godbolt.org/z/hc18qhfv1), so that part shouldn't matter.
I just looked at everything again with the understanding that adding a *copy* assignment operator doesn't have any impact on the generation of a copy constructor by the compiler (which frankly is bonkers), and I think this patch is correct without a doubt. The rules are just crazy though.
Can you rebase this onto main, make sure the CI is green and ship this? Please ping me again if you need to add any additional = deletes to this patch, I'll want to look again. This is kinda subtle and has the potential for a super subtle ABI break if gotten wrong.
libcxx/include/__bit_reference | ||
---|---|---|
168 | Since this only touches operator=, this can't affect TFPC-ness. | |
libcxx/include/map | ||
718 | Since this only touches operator=, this can't affect TFPC-ness. |
I'm not sure about this one, see question.