[NFC][Clang][Coverity] Fix Static Code Analysis Concerns with copy without assign

[Manna]

Summary

This patch adds missing assignment operator to the class which has user-defined copy/move constructor.

[Manna]

This comment has been deleted.

[Manna]

clang/include/clang/Sema/Sema.h

1789–1791

@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.

[aaronpuchert]

In several cases it's not completely obvious to me whether a copy assignment operator should or can be defined. But perhaps this doesn't need to be addressed right now: we seem to compile with -Wextra, which contains -Wdeprecated-copy. That should warn if a compiler-generated copy operation is used while another is user-declared.

clang/include/clang/AST/ASTContext.h

3212–3214

Why not just remove both? It seems to me the copy constructor doesn't behave different than the compiler-generated version.

clang/include/clang/Analysis/Analyses/Consumed.h

158–163

The copy constructor doesn't copy TmpMap, which means it's going to be empty afterwards, but if you leave it as-is on assignment, it keeps the previous value, which might be inconsistent with the newly assigned values to the other members. Perhaps the assignment operator should just be deleted?

clang/include/clang/Analysis/Analyses/ThreadSafetyUtil.h

243–246

I'd hope this is unused, also it probably doesn't do the right thing with NumRefs. Note that the copy constructor will initialize it with 1 due to the in-class initializer, while this is just going to keep the original value.

clang/include/clang/Analysis/Support/BumpVector.h

45–50

There should be no need to define this: declaring a move constructor should delete all other copy/move operations unless also explicitly declared, so this can't currently be used.

clang/include/clang/Sema/Lookup.h

660–661

These should also be implicitly deleted because of the user-declared move constructor.

clang/include/clang/Sema/Sema.h

1789–1791

These are also implicitly deleted. Some code styles want this explicitly spelled out, but I don't think ours does.

clang/lib/Sema/SemaAccess.cpp

203–207

Should this not set S.Target = nullptr like the move constructor? But perhaps this is also just unneeded.

[Manna]

Thank you @aaronpuchert for reviews and the comments.

[tahonermann]

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

clang/include/clang/Sema/Sema.h

1789–1791

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.

1790

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h

54

Here too; since a user-defined move constructor is declared, let's be explicit about move assignment.

clang/lib/CodeGen/CGDebugInfo.h

833–837

Good catch! Since the destructor uses CGF, the defaulted move assignment operator was wrong!

clang/lib/CodeGen/EHScopeStack.h

151

Here too; since a user-declared move constructor is present, let's be explicit about support for move assignment.

[Manna]

Thanks @tahonermann for your reviews and feedbacks. I have addressed your review comments.

clang/include/clang/Sema/Sema.h

1789–1791

I see! Thanks @tahonermann for the explanation

[aaronpuchert]

I'd probably abstain from explicitly deleting what is already implicitly deleted, but otherwise this looks good to me. Thanks!

clang/include/clang/Sema/Sema.h

1790

Being explicit is certainly a good thing, but the C++ committee wants to go into the direction of "copy/move operations are implicitly deleted if any of them or the destructor is user-declared." See depr.impldec. It is already partially the case, for example here. So why do need to spell something out explicitly when the language goes into the direction of being even more implicit?

[tahonermann]

clang/include/clang/Sema/Sema.h

1790

I have low expectations of that deprecation ever being acted on. In fact, there have been discussions about un-deprecating it because of the lack of such intent.

The motivation for deprecation is not based on a judgement of whether implicit or explicit is better, but rather based on a safety argument; if a copy constructor or destructor is user-declared, that is probably because some form of resource management is needed that won't be performed by the defaulted copy assignment operator. Thus, defining the implicit copy assignment operator as deleted would avoid the possibility of the compiler injecting bugs in the program.

The rules for when the copy vs assignment operators are implicitly defined as deleted can be hard to remember. I think being explicit is useful if only to indicate that the author thought about whether such operations should be provided. Since we don't have a principled reason for defining these operations as deleted, it might not be a bad idea to add a comment that states something like "The copy/move assignment operator is defined as deleted pending further motivation".

[Manna]

clang/include/clang/Sema/Sema.h

1790

Thank you @tahonermann for the comments and the updates about the deprecations discussion.

t might not be a bad idea to add a comment that states something like "The copy/move assignment operator is defined as deleted pending further motivation".

I like the idea of adding a comment since pending standard decision. I have updated my patch with the comments

[Manna]

Fix clang-format issues

[NoQ]

Static Analyzer changes look great, thanks!

[Manna]

In D150411#4354135, @NoQ wrote:

Static Analyzer changes look great, thanks!

Thank you @NoQ for reviews!

[tahonermann]

One last minor request and then I'm happy with this.

clang/include/clang/Sema/ParsedAttr.h

708–710

Let's explicitly delete the copy assignment operator too since the copy constructor is deleted.

[Manna]

clang/include/clang/Sema/ParsedAttr.h

708–710

@tahonermann , copy assignment operator has already been explicitly deleted at line no 701. I made this change part of https://reviews.llvm.org/D149718. It was a rebase issue, so it did show it before with my previous patch. I have fixed it now and added comment since the copy assignment was added explicitly as deleted.

[tahonermann]

Thanks @Manna, this looks good to me now!

clang/include/clang/Sema/ParsedAttr.h

708–710

Ah, perfect!

[Manna]

Thank you @tahonermann, @NoQ, @aaronpuchert for reviews!

[dblaikie]

(& I realize I'm a bit late to the party, but:

In several cases it's not completely obvious to me whether a copy assignment operator should or can be defined. But perhaps this doesn't need to be addressed right now: we seem to compile with -Wextra, which contains -Wdeprecated-copy. That should warn if a compiler-generated copy operation is used while another is user-declared.

The rules for when the copy vs assignment operators are implicitly defined as deleted can be hard to remember. I think being explicit is useful if only to indicate that the author thought about whether such operations should be provided. Since we don't have a principled reason for defining these operations as deleted, it might not be a bad idea to add a comment that states something like "The copy/move assignment operator is defined as deleted pending further motivation".

I'd also vote in favor of the "let it be implicitly deleted - we have the warnings enabled to catch it". But don't feel strongly enough to argue that chunks of this patch should be reverted)

clang/lib/Sema/SemaAccess.cpp

207

clang/include/clang/AST/ASTContext.h

private:
   void InitBuiltinType(CanQualType &R, BuiltinType::Kind K);
 
   class ObjCEncOptions {
     unsigned Bits;
 
     ObjCEncOptions(unsigned Bits) : Bits(Bits) {}
 
   public:
     ObjCEncOptions() : Bits(0) {}
-    ObjCEncOptions(const ObjCEncOptions &RHS) : Bits(RHS.Bits) {}
 
 #define OPT_LIST(V)                                                            \
   V(ExpandPointedToStructures, 0)                                              \
   V(ExpandStructures, 1)                                                       \
   V(IsOutermostType, 2)                                                        \
   V(EncodingProperty, 3)                                                       \
   V(IsStructField, 4)                                                          \
   V(EncodeBlockParameters, 5)                                                  \
   V(EncodeClassNames, 6)                                                       \
 

clang/include/clang/Analysis/Analyses/Consumed.h

protected:
     VarMapType VarMap;
     TmpMapType TmpMap;
 
   public:
     ConsumedStateMap() = default;
     ConsumedStateMap(const ConsumedStateMap &Other)
         : Reachable(Other.Reachable), From(Other.From), VarMap(Other.VarMap) {}
 
+    // The copy assignment operator is defined as deleted pending further
+    // motivation.
+    ConsumedStateMap &operator=(const ConsumedStateMap &) = delete;
+
     /// Warn if any of the parameters being tracked are not in the state
     /// they were declared to be in upon return from a function.
     void checkParamsForReturnTypestate(SourceLocation BlameLoc,
       ConsumedWarningsHandlerBase &WarningsHandler) const;
 
     /// Clear the TmpMap.
     void clearTemporaries();
 
     /// Get the consumed state of a given variable.
     ConsumedState getState(const VarDecl *Var) const;

clang/include/clang/Analysis/Analyses/ThreadSafetyTIL.h

 };
 
 /// Placeholder for expressions that cannot be represented in the TIL.
 class Undefined : public SExpr {
 public:
   Undefined(const Stmt *S = nullptr) : SExpr(COP_Undefined), Cstmt(S) {}
   Undefined(const Undefined &U) : SExpr(U), Cstmt(U.Cstmt) {}
 
+  // The copy assignment operator is defined as deleted pending further
+  // motivation.
+  Undefined &operator=(const Undefined &) = delete;
+
   static bool classof(const SExpr *E) { return E->opcode() == COP_Undefined; }
 
   template <class V>
   typename V::R_SExpr traverse(V &Vs, typename V::R_Ctx Ctx) {
     return Vs.reduceUndefined(*this);
   }
 
   template <class C>

 
 // Derived class for literal values, which stores the actual value.
 template<class T>
 class LiteralT : public Literal {
 public:
   LiteralT(T Dat) : Literal(ValueType::getValueType<T>()), Val(Dat) {}
   LiteralT(const LiteralT<T> &L) : Literal(L), Val(L.Val) {}
 
+  // The copy assignment operator is defined as deleted pending further
+  // motivation.
+  LiteralT &operator=(const LiteralT<T> &) = delete;
+
   T value() const { return Val;}
   T& value() { return Val; }
 
 private:
   T Val;
 };
 
 template <class V>

clang/include/clang/Analysis/Analyses/ThreadSafetyUtil.h

 class CopyOnWriteVector {
   class VectorData {
   public:
     unsigned NumRefs = 1;
     std::vector<T> Vect;
 
     VectorData() = default;
     VectorData(const VectorData &VD) : Vect(VD.Vect) {}
+
+    // The copy assignment operator is defined as deleted pending further
+    // motivation.
+    VectorData &operator=(const VectorData &) = delete;
   };
 
 public:
   CopyOnWriteVector() = default;
   CopyOnWriteVector(CopyOnWriteVector &&V) : Data(V.Data) { V.Data = nullptr; }
 
   CopyOnWriteVector &operator=(CopyOnWriteVector &&V) {
     destroy();

clang/include/clang/Analysis/Support/BumpVector.h

public:
   /// and destroys it when the BumpVectorContext object is destroyed.
   BumpVectorContext() : Alloc(new llvm::BumpPtrAllocator(), 1) {}
 
   BumpVectorContext(BumpVectorContext &&Other) : Alloc(Other.Alloc) {
     Other.Alloc.setInt(false);
     Other.Alloc.setPointer(nullptr);
   }
 
+  // The move assignment operator is defined as deleted pending further
+  // motivation.
+  BumpVectorContext &operator=(BumpVectorContext &&) = delete;
+
+  // The copy constrcutor and copy assignment operator is defined as deleted
+  // pending further motivation.
+  BumpVectorContext(const BumpVectorContext &) = delete;
+  BumpVectorContext &operator=(const BumpVectorContext &) = delete;
+
   /// Construct a new BumpVectorContext that reuses an existing
   /// BumpPtrAllocator.  This BumpPtrAllocator is not destroyed when the
   /// BumpVectorContext object is destroyed.
   BumpVectorContext(llvm::BumpPtrAllocator &A) : Alloc(&A, 0) {}
 
   ~BumpVectorContext() {
     if (Alloc.getInt())
       delete Alloc.getPointer();

clang/include/clang/Rewrite/Core/RewriteRope.h

class RewriteRope {
   llvm::IntrusiveRefCntPtr<RopeRefCountString> AllocBuffer;
   enum { AllocChunkSize = 4080 };
   unsigned AllocOffs = AllocChunkSize;
 
 public:
   RewriteRope() = default;
   RewriteRope(const RewriteRope &RHS) : Chunks(RHS.Chunks) {}
 
+  // The copy assignment operator is defined as deleted pending further
+  // motivation.
+  RewriteRope &operator=(const RewriteRope &) = delete;
+
   using iterator = RopePieceBTree::iterator;
   using const_iterator = RopePieceBTree::iterator;
 
   iterator begin() const { return Chunks.begin(); }
   iterator end() const { return Chunks.end(); }
   unsigned size() const { return Chunks.size(); }
 
   void clear() {

clang/include/clang/Sema/Lookup.h

public:
 
   public:
     Filter(Filter &&F)
         : Results(F.Results), I(F.I), Changed(F.Changed),
           CalledDone(F.CalledDone) {
       F.CalledDone = true;
     }
 
+    // The move assignment operator is defined as deleted pending
+    // further motivation.
+    Filter &operator=(Filter &&) = delete;
+
+    // The copy constrcutor and copy assignment operator is defined as deleted
+    // pending further motivation.
+    Filter(const Filter &) = delete;
+    Filter &operator=(const Filter &) = delete;
+
     ~Filter() {
       assert(CalledDone &&
              "LookupResult::Filter destroyed without done() call");
     }
 
     bool hasNext() const {
       return I != Results.end();
     }

clang/include/clang/Sema/ParsedAttr.h

class AttributePool {
 
   void takePool(AttributePool &pool);
 
 public:
   /// Create a new pool for a factory.
   AttributePool(AttributeFactory &factory) : Factory(factory) {}
 
   AttributePool(const AttributePool &) = delete;
+  // The copy assignment operator is defined as deleted pending further
+  // motivation.
   AttributePool &operator=(const AttributePool &) = delete;
 
   ~AttributePool() { Factory.reclaimPool(*this); }
 
   /// Move the given pool's allocations to this pool.
   AttributePool(AttributePool &&pool) = default;
 
+  // The move assignment operator is defined as deleted pending further
+  // motivation.
+  AttributePool &operator=(AttributePool &&pool) = delete;
+
   AttributeFactory &getFactory() const { return Factory; }
 
   void clear() {
     Factory.reclaimPool(*this);
     Attrs.clear();
   }
 
   /// Take the given pool's allocations and add them to this pool.

clang/include/clang/Sema/Sema.h

enum Kind {
       /// it's attached to is codegen'ed.  Also emit a call stack as with
       /// K_ImmediateWithCallStack.
       K_Deferred
     };
 
     SemaDiagnosticBuilder(Kind K, SourceLocation Loc, unsigned DiagID,
                           const FunctionDecl *Fn, Sema &S);
     SemaDiagnosticBuilder(SemaDiagnosticBuilder &&D);
     SemaDiagnosticBuilder(const SemaDiagnosticBuilder &) = default;
+
+    // The copy and move assignment operator is defined as deleted pending
+    // further motivation.
+    SemaDiagnosticBuilder &operator=(const SemaDiagnosticBuilder &) = delete;
+    SemaDiagnosticBuilder &operator=(SemaDiagnosticBuilder &&) = delete;
+
     ~SemaDiagnosticBuilder();
 
     bool isImmediate() const { return ImmediateDiag.has_value(); }
 
     /// Convertible to bool: True if we immediately emitted an error, false if
     /// we didn't emit an error or we created a deferred error.
     ///
     /// Example usage:

clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h

 using PathDiagnosticPieceRef = std::shared_ptr<PathDiagnosticPiece>;
 
 /// BugReporterVisitors are used to add custom diagnostics along a path.
 class BugReporterVisitor : public llvm::FoldingSetNode {
 public:
   BugReporterVisitor() = default;
   BugReporterVisitor(const BugReporterVisitor &) = default;
   BugReporterVisitor(BugReporterVisitor &&) {}
+
+  // The copy and move assignment operator is defined as deleted pending further
+  // motivation.
+  BugReporterVisitor &operator=(const BugReporterVisitor &) = delete;
+  BugReporterVisitor &operator=(BugReporterVisitor &&) = delete;
+
   virtual ~BugReporterVisitor();
 
   /// Return a diagnostic piece which should be associated with the
   /// given node.
   /// Note that this function does *not* get run on the very last node
   /// of the report, as the PathDiagnosticPiece associated with the
   /// last node should be unique.
   /// Use \ref getEndPath to customize the note associated with the report

clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h

 protected:
   ~SymbolVisitor() = default;
 
 public:
   SymbolVisitor() = default;
   SymbolVisitor(const SymbolVisitor &) = default;
   SymbolVisitor(SymbolVisitor &&) {}
 
+  // The copy and move assignment operator is defined as deleted pending further
+  // motivation.
+  SymbolVisitor &operator=(const SymbolVisitor &) = delete;
+  SymbolVisitor &operator=(SymbolVisitor &&) = delete;
+
   /// A visitor method invoked by ProgramStateManager::scanReachableSymbols.
   ///
   /// The method returns \c true if symbols should continue be scanned and \c
   /// false otherwise.
   virtual bool VisitSymbol(SymbolRef sym) = 0;
   virtual bool VisitMemRegion(const MemRegion *) { return true; }
 };
 
 } // namespace ento
 
 } // namespace clang
 
 #endif // LLVM_CLANG_STATICANALYZER_CORE_PATHSENSITIVE_SYMBOLMANAGER_H

clang/lib/CodeGen/CGDebugInfo.h

 public:
   /// Set the location to the (valid) TemporaryLocation.
   ApplyDebugLocation(CodeGenFunction &CGF, SourceLocation TemporaryLocation);
   ApplyDebugLocation(CodeGenFunction &CGF, const Expr *E);
   ApplyDebugLocation(CodeGenFunction &CGF, llvm::DebugLoc Loc);
   ApplyDebugLocation(ApplyDebugLocation &&Other) : CGF(Other.CGF) {
     Other.CGF = nullptr;
   }
-  ApplyDebugLocation &operator=(ApplyDebugLocation &&) = default;
+
+  // Define copy assignment operator.
+  ApplyDebugLocation &operator=(ApplyDebugLocation &&Other) {
+    CGF = Other.CGF;
+    Other.CGF = nullptr;
+    return *this;
+  }
 
   ~ApplyDebugLocation();
 
   /// Apply TemporaryLocation if it is valid. Otherwise switch
   /// to an artificial debug location that has a valid scope, but no
   /// line information.
   ///
   /// Artificial locations are useful when emitting compiler-generated

clang/lib/CodeGen/EHScopeStack.h

class Cleanup {
     virtual void anchor();
 
   protected:
     ~Cleanup() = default;
 
   public:
     Cleanup(const Cleanup &) = default;
     Cleanup(Cleanup &&) {}
+
+    // The copy and move assignment operator is defined as deleted pending
+    // further motivation.
+    Cleanup &operator=(const Cleanup &) = delete;
+    Cleanup &operator=(Cleanup &&) = delete;
+
     Cleanup() = default;
 
     virtual bool isRedundantBeforeReturn() { return false; }
 
     /// Generation flags.
     class Flags {
       enum {
         F_IsForEH = 0x1,

clang/lib/Sema/SemaAccess.cpp

struct AccessTarget : public AccessedEntity {
   }
 
   class SavedInstanceContext {
   public:
     SavedInstanceContext(SavedInstanceContext &&S)
         : Target(S.Target), Has(S.Has) {
       S.Target = nullptr;
     }
+
+    // The move assignment operator is defined as deleted pending further
+    // motivation.
+    SavedInstanceContext &operator=(SavedInstanceContext &&) = delete;
+
+    // The copy constrcutor and copy assignment operator is defined as deleted
+    // pending further motivation.
+    SavedInstanceContext(const SavedInstanceContext &) = delete;
+    SavedInstanceContext &operator=(const SavedInstanceContext &) = delete;
+
     ~SavedInstanceContext() {
       if (Target)
         Target->HasInstanceContext = Has;
     }
 
   private:
     friend struct AccessTarget;
     explicit SavedInstanceContext(AccessTarget &Target)

clang/utils/TableGen/ClangDiagnosticsEmitter.cpp

struct DiagText {
     int parseModifier(StringRef &) const;
 
   public:
     DiagText(DiagText &&O) noexcept
         : Builder(O.Builder), AllocatedPieces(std::move(O.AllocatedPieces)),
           Root(O.Root) {
       O.Root = nullptr;
     }
+    // The move assignment operator is defined as deleted pending further
+    // motivation.
+    DiagText &operator=(DiagText &&) = delete;
+
+    // The copy constrcutor and copy assignment operator is defined as deleted
+    // pending further motivation.
+    DiagText(const DiagText &) = delete;
+    DiagText &operator=(const DiagText &) = delete;
 
     ~DiagText() {
       for (Piece *P : AllocatedPieces)
         delete P;
     }
   };
 
 private: