[cfe-commits] r169670 - in /cfe/trunk: include/clang/AST/DeclCXX.h lib/AST/DeclCXX.cpp lib/Sema/SemaDeclCXX.cpp

Hi Richard,

We have an AST visitor that used the now-removed
getCopyAssignmentOperator() to collect type info for all members,
including implicit ones (see the IWYU project [1]).

Your commit comment hints that there may be more than one such
operator now... So is there a way to enumerate them? Or can we run
over all method decls and inspect them somehow?

This AST visitor is code we inherited and I'm not intimately familiar
with the flow -- maybe there's a better way to do this? There are
comments that suggest the RecursiveASTVisitor does not traverse
implicitly generated methods, maybe that's no longer true?

Thanks,
- Kim

[1] http://code.google.com/p/include-what-you-use/

Hi Richard,

We have an AST visitor that used the now-removed
getCopyAssignmentOperator() to collect type info for all members,
including implicit ones (see the IWYU project [1]).

Your commit comment hints that there may be more than one such
operator now… So is there a way to enumerate them? Or can we run
over all method decls and inspect them somehow?

This AST visitor is code we inherited and I’m not intimately familiar
with the flow – maybe there’s a better way to do this? There are
comments that suggest the RecursiveASTVisitor does not traverse
implicitly generated methods, maybe that’s no longer true?

Thanks,

  • Kim

[1] http://code.google.com/p/include-what-you-use/

I am not too well versed on the AST, but I can explain what multiple copy constructors are.

The following four methods are copy constructors:

Foo(Foo&);
Foo(Foo const&);
Foo(Foo volatile&);
Foo(Foo const volatile&);

– Matthieu

Hi Matthieu,

(+cfe-dev)

And, to answer one of my questions;

Hi Richard, all,

Here's what I finally settled on;

    // Check copy and move assignment operators.
    for (CXXRecordDecl::method_iterator it = decl->method_begin();
         it != decl->method_end(); ++it) {
      bool isAssignmentOperator = it->isCopyAssignmentOperator() ||
                                  it->isMoveAssignmentOperator();
      if (isAssignmentOperator && it->isImplicit()) {
          something(*it)
      }
    }

I think that's equivalent to our previous;

    if (hasImplicitDeclaredCopyAssignment) {
      something(decl->getCopyAssignmentOperator(true));
      something(decl->getCopyAssignmentOperator(false));
    }

Right?

Thanks,
- Kim

... oh, except it also handles move assignment operators, which is nice.

- K

It also only calls 'something' on the implicit copy assignment operator once :slight_smile:

Hi Richard,

I think that's equivalent to our previous;

    if (hasImplicitDeclaredCopyAssignment) {
      something(decl->getCopyAssignmentOperator(true));
      something(decl->getCopyAssignmentOperator(false));
    }

... oh, except it also handles move assignment operators, which is nice.

It also only calls 'something' on the implicit copy assignment operator once :slight_smile:

The new one, you mean?

I think the old one (above) called 'something' for;

T& operator=(const T& t);
T& operator=(T& t);

whereas the new one should call it for any of the below that exist:

T& operator=(const T& t);
T& operator=(const volatile T& t);
T& operator=(T& t);
T& operator=(volatile T& t);
T& operator=(T&& t);

I think that preserves the behavior we had and makes it more correct,
even if I'm not entirely sure why this is done. :-/

Thanks,
- Kim

There can only be one implicit copy assignment operator, and if there
is an implicit one, there are no explicit ones. The former code would
call 'something' twice, passing the same function both times...

Hi Richard,

I think the old one (above) called 'something' for;

T& operator=(const T& t);
T& operator=(T& t);

whereas the new one should call it for any of the below that exist:

T& operator=(const T& t);
T& operator=(const volatile T& t);
T& operator=(T& t);
T& operator=(volatile T& t);
T& operator=(T&& t);

I think that preserves the behavior we had and makes it more correct,
even if I'm not entirely sure why this is done. :-/

There can only be one implicit copy assignment operator, and if there
is an implicit one, there are no explicit ones. The former code would
call 'something' twice, passing the same function both times...

Thanks, it's getting clearer.

The former code passed true/false to get at different const-ness,
probably according to the rule described here [1] -- sometimes the
implicit assignment operator will take a const-ref arg and sometimes
it will take a non-const-ref arg.

It looks like getCopyAssignmentOperator returned NULL if we asked for
an overload that didn't exist, so I think the only reason this worked
was because the original code was really;

      something(decl->getCopyAssignmentOperator(true)) || // <--- note: OR
      something(decl->getCopyAssignmentOperator(false));

Since the non-const case is by far the most common, it short-circuited
out before it could attempt to run something(NULL).

Anyway, now I understand how to phrase this under the new
implementation, thanks a lot!

- Kim

[1] http://en.cppreference.com/w/cpp/language/as_operator