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

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

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.



================
Comment at: clang/include/clang/AST/ASTContext.h:3213-3218
     ObjCEncOptions(const ObjCEncOptions &RHS) : Bits(RHS.Bits) {}
 
+    ObjCEncOptions &operator=(const ObjCEncOptions &RHS) {
+      Bits = RHS.Bits;
+      return *this;
+    }
----------------
Why not just remove both? It seems to me the copy constructor doesn't behave different than the compiler-generated version.


================
Comment at: clang/include/clang/Analysis/Analyses/Consumed.h:158-163
+    ConsumedStateMap &operator=(const ConsumedStateMap &Other) {
+      Reachable = Other.Reachable;
+      From = Other.From;
+      VarMap = Other.VarMap;
+      return *this;
+    }
----------------
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?


================
Comment at: clang/include/clang/Analysis/Analyses/ThreadSafetyUtil.h:243-246
+    VectorData &operator=(const VectorData &VD) {
+      Vect = VD.Vect;
+      return *this;
+    }
----------------
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.


================
Comment at: clang/include/clang/Analysis/Support/BumpVector.h:45-50
+  BumpVectorContext &operator=(BumpVectorContext &&Other) {
+    Alloc = Other.Alloc;
+    Other.Alloc.setInt(false);
+    Other.Alloc.setPointer(nullptr);
+    return *this;
+  }
----------------
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.


================
Comment at: clang/include/clang/Sema/Lookup.h:660-661
 
+    Filter(const Filter &) = delete;
+    Filter &operator=(const Filter &) = delete;
+
----------------
These should also be implicitly deleted because of the user-declared move constructor.


================
Comment at: clang/include/clang/Sema/Sema.h:1789-1791
+    SemaDiagnosticBuilder &operator=(SemaDiagnosticBuilder &&D) = delete;
     SemaDiagnosticBuilder(const SemaDiagnosticBuilder &) = default;
+    SemaDiagnosticBuilder &operator=(const SemaDiagnosticBuilder &) = delete;
----------------
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.


================
Comment at: clang/lib/Sema/SemaAccess.cpp:203-207
+    SavedInstanceContext &operator=(SavedInstanceContext &&S) {
+      Target = S.Target;
+      Has = S.Has;
+      return *this;
+    }
----------------
Should this not set `S.Target = nullptr` like the move constructor? But perhaps this is also just unneeded.


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

https://reviews.llvm.org/D150411