This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by Manna on May 2 2023, 7:37 PM.

Details

Summary

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

Diff Detail

Event Timeline

Manna created this revision.May 2 2023, 7:37 PM
Herald added a project: Restricted Project. · View Herald Transcript
Manna requested review of this revision.May 2 2023, 7:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2023, 7:37 PM
Manna edited the summary of this revision. (Show Details)May 3 2023, 5:25 AM
Manna updated this revision to Diff 519331.May 3 2023, 7:04 PM

Fix clang-format errors and typo in comments

tahonermann requested changes to this revision.May 5 2023, 2:37 PM

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.

This revision now requires changes to proceed.May 5 2023, 2:37 PM
Manna updated this revision to Diff 520384.May 8 2023, 8:13 AM

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

Manna marked 6 inline comments as done.May 8 2023, 8:30 AM
Manna added inline comments.
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.

Manna edited the summary of this revision. (Show Details)May 8 2023, 8:31 AM
tahonermann requested changes to this revision.May 9 2023, 5:49 PM
tahonermann added inline comments.
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.

This revision now requires changes to proceed.May 9 2023, 5:49 PM
Manna updated this revision to Diff 521073.May 10 2023, 12:41 PM
Manna marked 2 inline comments as done.

I have updated patch

Manna added inline comments.May 10 2023, 12:45 PM
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 accepted this revision.May 10 2023, 3:41 PM

Looks good. Thanks @Manna!

clang/include/clang/Sema/Sema.h
1786 ↗(On Diff #519331)

Ok, sounds good.

This revision is now accepted and ready to land.May 10 2023, 3:41 PM
Manna added a comment.May 10 2023, 3:43 PM

Looks good. Thanks @Manna!

Thank you @tahonermann for reviews!

This revision was automatically updated to reflect the committed changes.

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.

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 added a comment.May 15 2023, 7:20 AM

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

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!