rsmith added inline comments.

Comment at: include/clang/AST/DeclCXX.h:420
+    /// \brief True if this class has at least one non-deleted copy or move
+    /// constructor. That would allow passing it by registers.
rnk wrote:
> Isn't this "... at least one *trivial*, non-deleted copy or move 
> constructor..."?
Changed to:

    /// \brief True if this class can be passed in a non-address-preserving
    /// fashion (such as in registers) according to the C++ language rules.
    /// This does not imply anything about how the ABI in use will actually
    /// pass an object of this class.

Comment at: include/clang/AST/DeclCXX.h:827
+    return data().DefaultedCopyConstructorIsDeleted;
+  }
+  /// \brief \c true if a defaulted move constructor for this class would be
v.g.vassilev wrote:
> Is there a reason for not keeping the default (for the file) 1 empty line 
> between methods? Looks like if we add one new line before and after 
> `hasSimpleMoveAssignment` is will be all consistent.
Done. (I was following the local style, but you're right that we don't do this 
elsewhere in the class outside the `hasSimple` functions, excluding groups of 
methods that are much more closely tied together such as `*_begin`/`*_end`.)

Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:836
-    // If this is true, the implicit copy constructor that Sema would have
-    // created would not be deleted. FIXME: We should provide a more direct way
-    // for CodeGen to ask whether the constructor was deleted.
-    if (!RD->hasUserDeclaredCopyConstructor() &&
-        !RD->hasUserDeclaredMoveConstructor() &&
-        !RD->needsOverloadResolutionForMoveConstructor() &&
-        !RD->hasUserDeclaredMoveAssignment() &&
-        !RD->needsOverloadResolutionForMoveAssignment())
-      return RAA_Default;
-    // Otherwise, Sema should have created an implicit copy constructor if
-    // needed.
-    assert(!RD->needsImplicitCopyConstructor());
-    // We have to make sure the trivial copy constructor isn't deleted.
-    for (const CXXConstructorDecl *CD : RD->ctors()) {
-      if (CD->isCopyConstructor()) {
-        assert(CD->isTrivial());
-        // We had at least one undeleted trivial copy ctor.  Return directly.
-        if (!CD->isDeleted())
-          return RAA_Default;
+    // Win64 passes objects with non-deleted, non-trivial copy ctors 
+    //
rnk wrote:
> This doesn't seem to match what we've computing, and it doesn't seem quite 
> right. MSVC will pass a class with deleted, trivial copy ctors indirectly. 
> Would it be correct to rephrase like this?
> "If RD has at least one trivial, non-deleted copy constructor, it is passed 
> directly. Otherwise, it is passed indirectly."
You're right. I think "it is passed directly" is overspecifying, though, so how 

    // If a class has at least one non-deleted, trivial copy constructor, it
    // is passed according to the C ABI. Otherwise, it is passed indirectly.

Comment at: lib/Sema/SemaDeclCXX.cpp:5731
+/// registers, per C++ [class.temporary]p3.
+static bool computeCanPassInRegisters(Sema &S, CXXRecordDecl *D) {
+  if (D->isDependentType() || D->isInvalidDecl())
v.g.vassilev wrote:
> It would be very useful if we somehow assert if this function is called 
> before the class triviality is computed?
Many of the functions we unconditionally call below will assert if the class 
does not have a complete definition (eg, `needsImplicitCopyConstructor`).

Comment at: test/CodeGenCXX/uncopyable-args.cpp:101
+// In MSVC 2013, the copy ctor is not deleted by a move assignment. In MSVC 
2015, it is.
+// WIN64-18-LABEL: declare void @"\01?foo@implicitly_deleted@@YAXUA@1@@Z"(i64
rnk wrote:
> Oh dear. :(
Can you check that MSVC 2013 is compatible with the code we produce here? (I've 
checked 2015 passes this indirectly on Compiler Explorer.)


cfe-commits mailing list

Reply via email to