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