Quuxplusone added inline comments.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2942
+ "not %select{move-constructible|destructible}2"
+ >;
+
----------------
> 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.
================
Comment at: include/clang/Sema/Sema.h:4304
+ bool IsTriviallyRelocatableType(QualType QT) const;
+
----------------
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.
================
Comment at: lib/Sema/SemaDeclCXX.cpp:6066
+ if (M->hasAttr<FinalAttr>() || Record->hasAttr<FinalAttr>()) {
+ // Consider removing this case to simplify the Standard wording.
+ } else {
----------------
Rakete1111 wrote:
> This should be a `// TODO: ...`. Is this comment really appropriate? The
> intended audience isn't compiler writers I think.
Similar to the `//puts` comments, this comment is appropriate for me but not
for Clang. :) Will replace with a comment containing the actual proposed
wording.
================
Comment at: lib/Sema/SemaDeclCXX.cpp:6157
+
+ if (getLangOpts().CPlusPlus11 &&
+ !Record->hasAttr<TriviallyRelocatableAttr>() &&
----------------
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.
================
Comment at: lib/Sema/SemaDeclCXX.cpp:6158
+ if (getLangOpts().CPlusPlus11 &&
+ !Record->hasAttr<TriviallyRelocatableAttr>() &&
+ Record->isTriviallyRelocatable()) {
----------------
Rakete1111 wrote:
> 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.
I just thought that checking the presence of the attribute would be cheaper
than doing these lookups, so I was trying to short-circuit it. However, you are
correct for two reasons:
- The check passes 99.99% of the time anyway, so it's *everyone* paying a small
cost just so that the 0.01% can be a bit faster. This is a bad tradeoff.
- Skipping the check when the attribute is present is actually incorrect, in
the case that the type is not relocatable and the attribute is going to be
dropped. (It was not dropped in this revision. It will be dropped in the next
revision.) In that case, even with the attribute, we still want to execute this
codepath.
================
Comment at: lib/Sema/SemaDeclCXX.cpp:6656
SetDeclDeleted(MD, MD->getLocation());
+ if (CSM == CXXMoveConstructor || CSM == CXXDestructor) {
+ //puts("because 6646");
----------------
Rakete1111 wrote:
> You don't actually need those three lines. This is already handled in
> `CheckCompletedCXXClass`.
Confirmed. Nice!
================
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}}
----------------
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 )
================
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");
+
----------------
>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 &&);
^
```
================
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'}}
----------------
Rakete1111 wrote:
> This is the right place for regression tests. Add them to
> test/Parser/cxx-class.cpp.
I believe both of these cases are already covered by other tests in the suite
(in fact that's how I found they were bad); I just wanted to have the known
trouble spots tested in this single file for my own convenience. I'll remove
these two "regression tests."
Repository:
rC Clang
https://reviews.llvm.org/D50119
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
