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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete1111 added a comment.

- 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.

- Please run your code through clang-format.

- Might be useful to add a note explaining why the type isn't trivially relocatable isn't of the general "because it is not destructible".



================
Comment at: include/clang/Sema/Sema.h:4304
 
+  bool IsTriviallyRelocatableType(QualType QT) const;
+
----------------
Any reason why this is a free function? Should be a member function of `QualType`.


================
Comment at: lib/AST/DeclCXX.cpp:283
+    if (Base->isVirtual() || !BaseClassDecl->isTriviallyRelocatable()) {
+      //puts("because 283");
+      setIsNotNaturallyTriviallyRelocatable();
----------------
Lingering debug message? :) There are many of them.

For a single expression, drop the braces of the if statement.


================
Comment at: lib/Sema/SemaDeclCXX.cpp:6066
+        if (M->hasAttr<FinalAttr>() || Record->hasAttr<FinalAttr>()) {
+          // Consider removing this case to simplify the Standard wording.
+        } else {
----------------
This should be a `// TODO: ...`. Is this comment really appropriate? The intended audience isn't compiler writers I think.


================
Comment at: lib/Sema/SemaDeclCXX.cpp:6157
+
+  if (getLangOpts().CPlusPlus11 &&
+      !Record->hasAttr<TriviallyRelocatableAttr>() &&
----------------
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.


================
Comment at: lib/Sema/SemaDeclCXX.cpp:6158
+  if (getLangOpts().CPlusPlus11 &&
+      !Record->hasAttr<TriviallyRelocatableAttr>() &&
+      Record->isTriviallyRelocatable()) {
----------------
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.


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


================
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}}
----------------
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.


================
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'}}
----------------
This is the right place for regression tests. Add them to test/Parser/cxx-class.cpp.


Repository:
  rC Clang

https://reviews.llvm.org/D50119