[cfe-commits] r74205 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/Sema.h lib/Sema/SemaDeclCXX.cpp lib/Sema/SemaExpr.cpp

[Chris Lattner]

On Jun 25, 2009, at 2:45 PM, Fariborz Jahanian wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=74205&view=rev
> Log:
> Patch to diagnose and Mark use of implicit default assignment  
> operator.

Hi Fariborz,

I don't have any meaningful comments about the semantics of this patch  
(though a testcase would be nice), a couple of picky things: :)

> +++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp Thu Jun 25 16:45:19 2009
> @@ -1942,6 +1942,94 @@
>     Constructor->setUsed();
> }
>
> +void Sema::DefineImplicitOverloadedAssign(SourceLocation  
> CurrentLocation,
> +                                          CXXMethodDecl  
> *MethodDecl) {
> +  assert((MethodDecl->isImplicit() && MethodDecl- 
> >isOverloadedOperator() &&
> +          MethodDecl->getOverloadedOperator() == OO_Equal &&
> +          !MethodDecl->isUsed()) &&
> +         "DefineImplicitOverloadedAssign - call it for implicit  
> assignment op");
> +
> +  CXXRecordDecl *ClassDecl
> +    = cast<CXXRecordDecl>(MethodDecl->getDeclContext());
> +  assert(ClassDecl && "DefineImplicitOverloadedAssign - invalid  
> constructor");

"cast" will abort it the pointer is null, so the assert isn't needed.   
If something can be null, you have to use "cast_or_null".

> +  // C++[class.copy] p210
> +  // Before the implicitly-declared copy assignment operator for a  
> class is
> +  // implicitly defined, all implicitly-declared copy assignment  
> operators
> +  // for its direct base classes and its nonstatic data members  
> shall have
> +  // been implicitly defined.
> +  bool err = false;

> +  for (CXXRecordDecl::base_class_iterator Base = ClassDecl- 
> >bases_begin();
> +       Base != ClassDecl->bases_end(); ++Base) {

You've been learning from Doug very well I see :).  The problem with  
this loop is that it evaluates ClassDecl->bases_end() every time  
through the loop.  In cases where the end iterator isn't changing (the  
container isn't being mutated by the loop), it is better to compute it  
once and refer to the pre-computed value.  Something like this:

> +  for (CXXRecordDecl::base_class_iterator Base = ClassDecl- 
> >bases_begin(),
> +       BaseE = ClassDecl->bases_end(); Base != BaseE; ++Base) {

This has two advantages: 1) it is slightly more efficient,  
particularly if the expression is more complex than "ClassDecl". 2) it  
makes it really obvious to the reader of the code that the container  
isn't being mutated.  Whenever I see code that evaluates end every  
time I assume that it is because the container is being mutated.

Please fix this in any cases that you come across, there are two in  
this patch, the other one is:

> +  for (CXXRecordDecl::field_iterator Field = ClassDecl- 
> >field_begin(Context);
> +       Field != ClassDecl->field_end(Context);
> +       ++Field) {

Here.

You're doing great stuff, I don't mean to pick on you at all.  Doug  
and I have had long "discussions" about this in the past too :)

-Chris