tahonermann requested changes to this revision.
tahonermann added a comment.
This revision now requires changes to proceed.

I added a few comments and suggested edits, but this is mostly looking good.



================
Comment at: clang/include/clang/Sema/Sema.h:1790
     SemaDiagnosticBuilder(const SemaDiagnosticBuilder &) = default;
+    SemaDiagnosticBuilder &operator=(const SemaDiagnosticBuilder &) = delete;
     ~SemaDiagnosticBuilder();
----------------



================
Comment at: clang/include/clang/Sema/Sema.h:1789-1791
+    SemaDiagnosticBuilder &operator=(SemaDiagnosticBuilder &&D) = delete;
     SemaDiagnosticBuilder(const SemaDiagnosticBuilder &) = default;
+    SemaDiagnosticBuilder &operator=(const SemaDiagnosticBuilder &) = delete;
----------------
aaronpuchert wrote:
> Manna wrote:
> > @tahonermann This is follow-up comments from 
> > https://reviews.llvm.org/D149718?id=519331#inline-1452044. 
> > 
> > >>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.
> > 
> > I tried to define move assignment operator in ` clang/lib/Sema/Sema.cpp` 
> > but it failed because class Sema has deleted implicit copy assignment 
> > operator.
> > 
> > ```
> > /// Sema - This implements semantic analysis and AST building for C.
> > class Sema final {
> >   Sema(const Sema &) = delete;
> >   void operator=(const Sema &) = delete;
> > ```
> > It seems like support for assignment is not desired, We probably need 
> > deleted copy/move assignment operator.
> > 
> These are also implicitly deleted. Some code styles want this explicitly 
> spelled out, but I don't think ours does.
> I tried to define move assignment operator in  clang/lib/Sema/Sema.cpp but it 
> failed because class Sema has deleted implicit copy assignment operator.

It is still permissible to define a move assignment operator if the implicit 
copy assignment operator is deleted. See https://godbolt.org/z/sGaWd9M44.

I think it is fine to disable support for assignment for this class pending use 
cases. But, since a move constructor is explicitly defined, we should also be 
explicit above move assignment. I added a suggested edit. Without that change, 
I think Coverity will continue to complain.


================
Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:54
   BugReporterVisitor(BugReporterVisitor &&) {}
+  BugReporterVisitor &operator=(const BugReporterVisitor &) = delete;
   virtual ~BugReporterVisitor();
----------------
Here too; since a user-defined move constructor is declared, let's be explicit 
about move assignment.


================
Comment at: clang/lib/CodeGen/CGDebugInfo.h:833-837
+  ApplyDebugLocation &operator=(ApplyDebugLocation &&Other) {
+    CGF = Other.CGF;
+    Other.CGF = nullptr;
+    return *this;
+  }
----------------
Good catch! Since the destructor uses `CGF`, the defaulted move assignment 
operator was wrong!


================
Comment at: clang/lib/CodeGen/EHScopeStack.h:151
     Cleanup(Cleanup &&) {}
+    Cleanup &operator=(const Cleanup &) = delete;
     Cleanup() = default;
----------------
Here too; since a user-declared move constructor is present, let's be explicit 
about support for move assignment.


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

https://reviews.llvm.org/D150411

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

Reply via email to