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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone 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
----------------
Rakete1111 wrote:
> What's this file? A mistake?
Yeah, it's pipework for the Compiler Explorer integration. I manually deleted it from the previously uploaded diff, but forgot to delete it this time. You can ignore it.


================
Comment at: include/clang/Sema/Sema.h:4304
 
+  bool IsTriviallyRelocatableType(QualType QT) const;
+
----------------
Rakete1111 wrote:
> 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.
Okay, moved!


================
Comment at: lib/Sema/SemaDeclAttr.cpp:6481
     break;
+  case ParsedAttr::AT_TriviallyRelocatable:
+    handleTriviallyRelocatableAttr(S, D, AL);
----------------
Rakete1111 wrote:
> Why is this attribute under "Microsoft Attributes"? ;)
I dunno, the random `//comments` seem pretty silly to me. There's nothing "Microsoft" about TrivialABI either (and no reason anyone should care, in this context, that EmptyBases or LayoutVersion are "Microsoft"). I just put it here because it's somewhat related to TrivialABI — and/or, to reduce code churn if anyone ever decides to Do The Right Thing and alphabetize these switch cases.


================
Comment at: lib/Sema/SemaDeclCXX.cpp:6187
+        Record->dropAttr<TriviallyRelocatableAttr>();
+      } else if (Record->needsImplicitMoveConstructor() &&
+                 Record->defaultedMoveConstructorIsDeleted()) {
----------------
Rakete1111 wrote:
> 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`.
Confirmed that this code is never hit; and removed. Just for my own information: you're saying that the call to `LookupSpecialMember` on line 6179, even though it's looking up the //destructor//, will actually cause all the `needsImplicitFootor` flags to get resolved? And so also I guess I should never have been looking at those flags directly; I should have handled this case by calling `LookupSpecialMember` like I do on line 6196. Is that basically correct?


Repository:
  rC Clang

https://reviews.llvm.org/D50119