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@-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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
