[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

Wed Aug 1 13:12:05 PDT 2018

Quuxplusone added inline comments.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2942
+  "not %select{move-constructible|destructible}2"
+  >;
+
----------------
> Might be useful to add a note explaining why the type isn't trivially relocatable instead of the general "because it is not destructible".

You mean, like, a series of notes pointing at "because its base class B is not destructible... because B's destructor is defined as deleted here"?  I agree that might be helpful, but since it only happens when the programmer is messing around with the attribute anyway, I wouldn't want to do anything too innovative or LoC-consuming. I'd cut and paste ~10 lines of code from somewhere else that already does something like that if you point me at it, but otherwise I think it's not worth the number of extra codepaths that would need to be tested.

================
Comment at: include/clang/Sema/Sema.h:4304

+  bool IsTriviallyRelocatableType(QualType QT) const;
+
----------------
Rakete1111 wrote:
> Any reason why this is a free function? Should be a member function of `QualType`.
`class QualType` is defined in `AST/`, whereas all the C++-specific TriviallyRelocatable stuff is currently confined to `Sema/`. My impression was that I should try to preserve that separation, even if it meant being ugly right here. I agree that this is a stupid hack, and would love to get rid of it, but I think I need some guidance as to how much mixing of `AST` and `Sema` is permitted.

================
Comment at: lib/Sema/SemaDeclCXX.cpp:6066
+        if (M->hasAttr<FinalAttr>() || Record->hasAttr<FinalAttr>()) {
+          // Consider removing this case to simplify the Standard wording.
+        } else {
----------------
Rakete1111 wrote:
> This should be a `// TODO: ...`. Is this comment really appropriate? The intended audience isn't compiler writers I think.
Similar to the `//puts` comments, this comment is appropriate for me but not for Clang. :) Will replace with a comment containing the actual proposed wording.

================
Comment at: lib/Sema/SemaDeclCXX.cpp:6157
+
+  if (getLangOpts().CPlusPlus11 &&
+      !Record->hasAttr<TriviallyRelocatableAttr>() &&
----------------
Rakete1111 wrote:
> This really just checks whether the type has defaulted copy constructor. If there was a move constructor, it would have been handled above. If the copy/move constructor is implicitly deleted, it would have been handled also above. Please simplify this accordingly.
Can you elaborate on this one? I assume you mean that some of lines 6157 through 6171 are superfluous, but I can't figure out which ones or how to simplify it.

================
Comment at: lib/Sema/SemaDeclCXX.cpp:6158
+  if (getLangOpts().CPlusPlus11 &&
+      !Record->hasAttr<TriviallyRelocatableAttr>() &&
+      Record->isTriviallyRelocatable()) {
----------------
Rakete1111 wrote:
> Why do you need to check whether the attribute is present or not? This is supposed to be whether the type is naturally trivially relocatable, so the presence of the attribute is not important.
I just thought that checking the presence of the attribute would be cheaper than doing these lookups, so I was trying to short-circuit it. However, you are correct for two reasons:
- The check passes 99.99% of the time anyway, so it's *everyone* paying a small cost just so that the 0.01% can be a bit faster. This is a bad tradeoff.
- Skipping the check when the attribute is present is actually incorrect, in the case that the type is not relocatable and the attribute is going to be dropped. (It was not dropped in this revision. It will be dropped in the next revision.) In that case, even with the attribute, we still want to execute this codepath.

================
Comment at: lib/Sema/SemaDeclCXX.cpp:6656
       SetDeclDeleted(MD, MD->getLocation());
+      if (CSM == CXXMoveConstructor || CSM == CXXDestructor) {
+        //puts("because 6646");
----------------
Rakete1111 wrote:
> You don't actually need those three lines. This is already handled in `CheckCompletedCXXClass`.
Confirmed. Nice!

================
Comment at: test/SemaCXX/trivially-relocatable.cpp:42
+struct A6;
+struct [[trivially_relocatable]] A6 {};
+// expected-error at -1{{type A6 declared 'trivially_relocatable' after its first declaration}}
----------------
Rakete1111 wrote:
> Why does this restriction exist? None of the existing attributes have it and I don't see why it would make sense to disallow this.
`[[noreturn]]` has it, and I am pretty sure that `[[trivial_abi]]` *ought* to have it, even though it currently doesn't. The intent is to make it harder for people to create ODR violations by declaring a type, using it in some way, and then saying "oh by the way this type was xxxxx all along."

```
struct S { S(S&&); ~S(); };
std::vector<S> vec;
struct [[trivially_relocatable]] S;  // ha ha, now you have to re-do all of vector's codegen!
```

Does that make sense as a reason it would be (mildly) beneficial to have this diagnostic?
You could still reasonably object to my assumptions that (A) the diagnostic is worth implementing in Clang and/or (B) the diagnostic is worth mandating in the Standard wording; but FWIW I do think it's very cheap both to implement and to specify.

(I will try to adjust the patch so that after the error is given, we remove the attribute from the class so as to make consistent any subsequent cascading errors. https://godbolt.org/g/nVRiow )

================
Comment at: test/SemaCXX/trivially-relocatable.cpp:429
+static_assert(__is_constructible(T23a, const T23a&), "");
+static_assert(!__is_trivially_relocatable(T23a), "because its copy operation is evil");
+
----------------
>There's a bug in your implementation:
>```
>struct X {
>  X &operator=(X &&);
>};
>static_assert(__is_trivially_relocatable(X)); // oops, fires!
>```
>X has a move constructor and a destructor, so it is trivially relocatable.

Actually, this `X` does *not* have a move constructor! And its copy constructor is implicitly deleted. So it is not relocatable, let alone trivially relocatable.
https://godbolt.org/g/ZsCqvd
```
<source>:15:7: error: call to implicitly-deleted copy constructor of 'X'
    X y(std::move(x));
      ^ ~~~~~~~~~~~~
<source>:10:6: note: copy constructor is implicitly deleted because 'X' has a user-declared move assignment operator
  X &operator=(X &&);
     ^
```

================
Comment at: test/SemaCXX/trivially-relocatable.cpp:471
+struct Incomplete; // expected-note {{forward declaration of 'Incomplete'}}
+struct Regression1 {
+  Incomplete i; // expected-error {{field has incomplete type 'Incomplete'}}
----------------
Rakete1111 wrote:
> This is the right place for regression tests. Add them to test/Parser/cxx-class.cpp.
I believe both of these cases are already covered by other tests in the suite (in fact that's how I found they were bad); I just wanted to have the known trouble spots tested in this single file for my own convenience. I'll remove these two "regression tests."

Repository:
  rC Clang

https://reviews.llvm.org/D50119