Manna added inline comments.

================
Comment at: clang/include/clang/Sema/Sema.h:1786
     SemaDiagnosticBuilder(SemaDiagnosticBuilder &&D);
+    SemaDiagnosticBuilder &operator=(SemaDiagnosticBuilder &&D);
     SemaDiagnosticBuilder(const SemaDiagnosticBuilder &) = default;
----------------
tahonermann wrote:
> Manna wrote:
> > tahonermann wrote:
> > > Since this class declares a move constructor, the declaration of the 
> > > implicit move assignment operator is inhibited and the implicitly 
> > > declared assignment operator is defined as deleted. This change declares 
> > > a move assignment operator but doesn't define it (perhaps `= delete` was 
> > > intended?). If support for assignment is not desired, then I think a 
> > > declaration of a deleted copy assignment operator is all that is needed 
> > > (matching the change made for `Strategy` below). Otherwise, I think a 
> > > defaulted copy assignment operator should be declared and a move 
> > > assignment operator should be defined that implements the same behavior 
> > > as the move constructor.
> > Thanks @tahonermann for the comments. 
> > 
> > >> think a defaulted copy assignment operator should be declared and a move 
> > >> assignment operator should be defined that implements the same behavior 
> > >> as the move constructor.
> > 
> > I have updated patch based on further analysis and my understanding. This 
> > seems reasonable to me.
> This change still declares a move assignment operator, but doesn't provide a 
> definition. The move constructor is implemented in `clang/lib/Sema/Sema.cpp`, 
> so I would expect to see the move assignment operator definition provided 
> there as well.
Thanks @tahonermann for the comments. I have removed `Sema.h`  change from the 
current patch. I will address it in a separate review pull request. Need to 
look into this more.  


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149718/new/

https://reviews.llvm.org/D149718

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to