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

Wed Aug 1 22:14:33 PDT 2018

Rakete1111 added inline comments.

================
Comment at: compiler-explorer-llvm-commit.sh:1
+# This is the commit of LLVM that we're currently based on.
+git reset --hard 1fa19f68007cd126a04448093c171f40e556087e
----------------
What's this file? A mistake?

================
Comment at: include/clang/AST/DeclCXX.h:471

+    /// True when this class's bases and fields are all trivially relocatable,
+    /// and the class itself has a defaulted move constructor and a defaulted
----------------
That comment misses a "or is a reference".

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2942
+  "not %select{move-constructible|destructible}2"
+  >;
+
----------------
Quuxplusone wrote:
> > 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.
Fair enough.

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

+  bool IsTriviallyRelocatableType(QualType QT) const;
+
----------------
Quuxplusone wrote:
> 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.
Nah, it's fine. There are lots of C++ specific things in AST/, because the AST nodes represent C++-y stuff. Trivially copyable is also part of `QualType`, even though it's C++ specific.

================
Comment at: lib/Sema/SemaDecl.cpp:15823
         CXXRecord->setImplicitDestructorIsDeleted();
+        CXXRecord->setIsNotNaturallyTriviallyRelocatable();
         SetDeclDeleted(Dtor, CXXRecord->getLocation());
----------------
You don't need this. This is already handled by `CheckCompleteCXXClass`.

================
Comment at: lib/Sema/SemaDeclAttr.cpp:6481
     break;
+  case ParsedAttr::AT_TriviallyRelocatable:
+    handleTriviallyRelocatableAttr(S, D, AL);
----------------
Why is this attribute under "Microsoft Attributes"? ;)

================
Comment at: lib/Sema/SemaDeclCXX.cpp:6166
+        if (SMOR.getKind() != SpecialMemberOverloadResult::Success ||
+            !SMOR.getMethod()->isDefaulted()) {
+          Record->setIsNotNaturallyTriviallyRelocatable();
----------------
Extra braces.

================
Comment at: lib/Sema/SemaDeclCXX.cpp:6187
+        Record->dropAttr<TriviallyRelocatableAttr>();
+      } else if (Record->needsImplicitMoveConstructor() &&
+                 Record->defaultedMoveConstructorIsDeleted()) {
----------------
This is dead code. `Record` never needs an implicit move constructor at this point, because either 1) it never did or 2) it was defined above by `LookupSpecialMember`.

================
Comment at: lib/Sema/SemaDeclCXX.cpp:12631
     ClassDecl->setImplicitMoveConstructorIsDeleted();
+    ClassDecl->setIsNotNaturallyTriviallyRelocatable();
     SetDeclDeleted(MoveConstructor, ClassLoc);
----------------
Same, already handled by `CheckCompleteCXXClass`.

================
Comment at: lib/Sema/SemaDeclCXX.cpp:6157
+
+  if (getLangOpts().CPlusPlus11 &&
+      !Record->hasAttr<TriviallyRelocatableAttr>() &&
----------------
Quuxplusone wrote:
> 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.
Actually, nvm. I was thinking of calling a few functions instead of call `LookupSpecialMember`, but I forgot that those functions can only work if there was previously a call to `LookupSpecialMember` :/.

================
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}}
----------------
Quuxplusone wrote:
> 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 )
Didn't know `[[noreturn]]` had it. Ah I see. Sold :)

================
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");
+
----------------
Quuxplusone wrote:
> >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 &&);
>      ^
> ```
Oops, you're right ofc. Sorry

Repository:
  rC Clang

https://reviews.llvm.org/D50119