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