tahonermann accepted this revision.
tahonermann added a comment.
This revision is now accepted and ready to land.
Looks good. Thanks @Manna!
================
Comment at: clang/include/clang/Sema/Sema.h:1786
SemaDiagnosticBuilder(SemaDiagnosticBuilder &&D);
+ SemaDiagnosticBuilder &operator=(SemaDiagnosticBuilder &&D);
SemaDiagnosticBuilder(const SemaDiagnosticBuilder &) = default;
----------------
Manna wrote:
> 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.
Ok, sounds good.
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
