[NFC][Clang] Fix Coverity issues of copy without assign

[Manna]

Summary

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

[Manna]

Fix clang-format errors and typo in comments

[tahonermann]

This looks good with one exception; I think the changes for class SemaDiagnosticBuilder are not what was intended.

clang/include/clang/Analysis/BodyFarm.h

41–46

This looks fine; move constructor and assignment declarations are inhibited.

clang/include/clang/Sema/ParsedAttr.h

704–705

This class has a move constructor, but the implicit declaration of a move assignment operator will be inhibited due to the presence of this and other special member functions. That means that an attempted move assignment will find the deleted copy assignment. That is ok; if support for move assignment is desired some day, it can be added then.

916–918

This looks fine; move constructor and assignment declarations are inhibited.

clang/include/clang/Sema/Sema.h

1786 ↗

(On Diff #519331)

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.

clang/lib/Analysis/UnsafeBufferUsage.cpp

764–767

This is ok. The implicit move assignment declaration is inhibited and since copy assignment is deleted, no assignment is supported. If move assignment is desired someday, it can be added.

clang/lib/Serialization/ASTWriterStmt.cpp

44–46

This looks fine; implicit move construction and assignment declarations are inhibited.

[Manna]

Thanks @tahonermann for reviews. I have addressed review comments and updated patches.

[Manna]

clang/include/clang/Sema/ParsedAttr.h

704–705

I have added defaulted move assignment operator which seems needed based on analysis with other bugs.

clang/include/clang/Sema/Sema.h

1786 ↗

(On Diff #519331)

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.

[tahonermann]

clang/include/clang/Sema/Sema.h

1786 ↗

(On Diff #519331)

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.

[Manna]

I have updated patch

[Manna]

clang/include/clang/Sema/Sema.h

1786 ↗

(On Diff #519331)

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.

[tahonermann]

Looks good. Thanks @Manna!

clang/include/clang/Sema/Sema.h

1786 ↗

(On Diff #519331)

Ok, sounds good.

[Manna]

In D149718#4333563, @tahonermann wrote:

Looks good. Thanks @Manna!

Thank you @tahonermann for reviews!

[uabelho]

Compiling with clang 15.0.5 I get the following warning/error with this patch:

../../clang/include/clang/Sema/ParsedAttr.h:705:18: error: explicitly defaulted move assignment operator is implicitly deleted [-Werror,-Wdefaulted-function-deleted]
  AttributePool &operator=(AttributePool &&pool) = default;
                 ^
../../clang/include/clang/Sema/ParsedAttr.h:674:21: note: move assignment operator of 'AttributePool' is implicitly deleted because field 'Factory' is of reference type 'clang::AttributeFactory &'
  AttributeFactory &Factory;
                    ^
1 error generated.

[uabelho]

In D149718#4341052, @uabelho wrote:

Compiling with clang 15.0.5 I get the following warning/error with this patch:

../../clang/include/clang/Sema/ParsedAttr.h:705:18: error: explicitly defaulted move assignment operator is implicitly deleted [-Werror,-Wdefaulted-function-deleted]
  AttributePool &operator=(AttributePool &&pool) = default;
                 ^
../../clang/include/clang/Sema/ParsedAttr.h:674:21: note: move assignment operator of 'AttributePool' is implicitly deleted because field 'Factory' is of reference type 'clang::AttributeFactory &'
  AttributeFactory &Factory;
                    ^
1 error generated.

Also seen in this buildbot:
https://lab.llvm.org/buildbot/#/builders/214/builds/7476

[Manna]

In D149718#4341384, @uabelho wrote:

In D149718#4341052, @uabelho wrote:

Compiling with clang 15.0.5 I get the following warning/error with this patch:

../../clang/include/clang/Sema/ParsedAttr.h:705:18: error: explicitly defaulted move assignment operator is implicitly deleted [-Werror,-Wdefaulted-function-deleted]
  AttributePool &operator=(AttributePool &&pool) = default;
                 ^
../../clang/include/clang/Sema/ParsedAttr.h:674:21: note: move assignment operator of 'AttributePool' is implicitly deleted because field 'Factory' is of reference type 'clang::AttributeFactory &'
  AttributeFactory &Factory;
                    ^
1 error generated.

Also seen in this buildbot:
https://lab.llvm.org/buildbot/#/builders/214/builds/7476

Thank you @uabelho for reporting. Build error is fixed by https://github.com/llvm/llvm-project/commit/292a6c1c2395f990bbde8d968825243e4fe9b954

[uabelho]

In D149718#4342320, @Manna wrote:

In D149718#4341384, @uabelho wrote:

In D149718#4341052, @uabelho wrote:

Compiling with clang 15.0.5 I get the following warning/error with this patch:

../../clang/include/clang/Sema/ParsedAttr.h:705:18: error: explicitly defaulted move assignment operator is implicitly deleted [-Werror,-Wdefaulted-function-deleted]
  AttributePool &operator=(AttributePool &&pool) = default;
                 ^
../../clang/include/clang/Sema/ParsedAttr.h:674:21: note: move assignment operator of 'AttributePool' is implicitly deleted because field 'Factory' is of reference type 'clang::AttributeFactory &'
  AttributeFactory &Factory;
                    ^
1 error generated.

Also seen in this buildbot:
https://lab.llvm.org/buildbot/#/builders/214/builds/7476

Thank you @uabelho for reporting. Build error is fixed by https://github.com/llvm/llvm-project/commit/292a6c1c2395f990bbde8d968825243e4fe9b954

Great, thanks!

clang/include/clang/Analysis/BodyFarm.h

public:
 
   /// Factory method for creating bodies for ordinary functions.
   Stmt *getBody(const FunctionDecl *D);
 
   /// Factory method for creating bodies for Objective-C properties.
   Stmt *getBody(const ObjCMethodDecl *D);
 
   /// Remove copy constructor to avoid accidental copying.
   BodyFarm(const BodyFarm &other) = delete;
 
+  /// Delete copy assignment operator.
+  BodyFarm &operator=(const BodyFarm &other) = delete;
+
 private:
   typedef llvm::DenseMap<const Decl *, std::optional<Stmt *>> BodyMap;
 
   ASTContext &C;
   BodyMap Bodies;
   CodeInjector *Injector;
 };
 } // namespace clang
 
 #endif

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;
+  AttributePool &operator=(const AttributePool &) = delete;
 
   ~AttributePool() { Factory.reclaimPool(*this); }
 
   /// Move the given pool's allocations to this pool.
   AttributePool(AttributePool &&pool) = default;
+  AttributePool &operator=(AttributePool &&pool) = default;
 
   AttributeFactory &getFactory() const { return Factory; }
 
   void clear() {
     Factory.reclaimPool(*this);
     Attrs.clear();
   }
 

 /// we don't differentiate between the various attribute syntaxes,
 /// which is basically silly.
 ///
 /// Right now this is a very lightweight container, but the expectation
 /// is that this will become significantly more serious.
 class ParsedAttributes : public ParsedAttributesView {
 public:
   ParsedAttributes(AttributeFactory &factory) : pool(factory) {}
   ParsedAttributes(const ParsedAttributes &) = delete;
+  ParsedAttributes &operator=(const ParsedAttributes &) = delete;
 
   AttributePool &getPool() const { return pool; }
 
   void takeAllFrom(ParsedAttributes &Other) {
     assert(&Other != this &&
            "ParsedAttributes can't take attributes from itself");
     addAll(Other.begin(), Other.end());
     Other.clearListOnly();
     pool.takeAllFrom(Other.pool);

clang/lib/Analysis/UnsafeBufferUsage.cpp

 
 private:
   using MapTy = llvm::DenseMap<const VarDecl *, Kind>;
 
   MapTy Map;
 
 public:
   Strategy() = default;
   Strategy(const Strategy &) = delete; // Let's avoid copies.
+  Strategy &operator=(const Strategy &) = delete;
   Strategy(Strategy &&) = default;
+  Strategy &operator=(Strategy &&) = default;
 
   void set(const VarDecl *VD, Kind K) { Map[VD] = K; }
 
   Kind lookup(const VarDecl *VD) const {
     auto I = Map.find(VD);
     if (I == Map.end())
       return Kind::Wontfix;
 

clang/lib/Serialization/ASTWriterStmt.cpp

class ASTStmtWriter : public StmtVisitor<ASTStmtWriter, void> {
     serialization::StmtCode Code;
     unsigned AbbrevToUse;
 
   public:
     ASTStmtWriter(ASTWriter &Writer, ASTWriter::RecordData &Record)
         : Writer(Writer), Record(Writer, Record),
           Code(serialization::STMT_NULL_PTR), AbbrevToUse(0) {}
 
     ASTStmtWriter(const ASTStmtWriter&) = delete;
+    ASTStmtWriter &operator=(const ASTStmtWriter &) = delete;
 
     uint64_t Emit() {
       assert(Code != serialization::STMT_NULL_PTR &&
              "unhandled sub-statement writing AST file");
       return Record.EmitStmt(Code, AbbrevToUse);
     }
 
     void AddTemplateKWAndArgsInfo(const ASTTemplateKWAndArgsInfo &ArgInfo,
                                   const TemplateArgumentLoc *Args);