Skip to content

Fix Clang-trunk warnings about special members deprecated in C++20.#5829

Merged
aardappel merged 1 commit intogoogle:masterfrom
Quuxplusone:assign-operators
Mar 23, 2020
Merged

Fix Clang-trunk warnings about special members deprecated in C++20.#5829
aardappel merged 1 commit intogoogle:masterfrom
Quuxplusone:assign-operators

Conversation

@Quuxplusone
Copy link
Copy Markdown
Contributor

@Quuxplusone Quuxplusone commented Mar 22, 2020

For example:

include/flatbuffers/reflection.h:365:8: error: definition of implicit copy
      constructor for 'pointer_inside_vector<flatbuffers::Table, unsigned char>'
      is deprecated because it has a user-declared copy assignment operator
      [-Werror,-Wdeprecated-copy]
  void operator=(const pointer_inside_vector &piv);
       ^

It's unclear why the old code wanted to declare a public operator=
without defining it; that just seems like a misunderstanding of the C++03 idiom
for deleting a member function. And anyway, we don't want to delete the
assignment operator; these are polymorphic types that do not follow value
semantics and nobody should ever be trying to copy them. So the simplest fix
is just to go back to the Rule of Zero: remove the declaration of operator=
and let the compiler do what it wanted to do originally anyway.
"The best code is no code."

Fixes #5649.

@Quuxplusone
Copy link
Copy Markdown
Contributor Author

@vglavnyy: I don't see any obvious way to add you as an "Assignee" and/or "Reviewer", but, please take a look.

Copy link
Copy Markdown
Contributor

@vglavnyy vglavnyy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with these changes.
Changed classes (structs) have reference & members (&vec_ or &fbb_) that block copy or move operations.
In every case operator= is unnecessary (public or private or delete).

: fbb_(_fbb) {
start_ = fbb_.StartTable();
}
TypeBuilder &operator=(const TypeBuilder &);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, flatbuffers::FlatBufferBuilder &fbb_; blocks copy and move even if there was an idea to make &operator= private.

For example:

    include/flatbuffers/reflection.h:365:8: error: definition of implicit copy
          constructor for 'pointer_inside_vector<flatbuffers::Table, unsigned char>'
          is deprecated because it has a user-declared copy assignment operator
          [-Werror,-Wdeprecated-copy]
      void operator=(const pointer_inside_vector &piv);
           ^

It's unclear why the old code wanted to declare a public `operator=`
without defining it; that just seems like a misunderstanding of the C++03 idiom
for deleting a member function. And anyway, we don't *want* to delete the
assignment operator; these are polymorphic types that do not follow value
semantics and nobody should ever be trying to copy them. So the simplest fix
is just to go back to the Rule of Zero: remove the declaration of `operator=`
and let the compiler do what it wanted to do originally anyway.
"The best code is no code."

Also, update the generated .h files.

Fixes #5649.
@Quuxplusone
Copy link
Copy Markdown
Contributor Author

Updated (including your suggested fix to reflection.cpp).

I think that it would probably be useful to investigate and eliminate all uses of FLATBUFFERS_DELETE_FUNC; it seems like it's been applied pretty haphazardly and again isn't really doing anything except "protecting" against a very specific logic bug that nobody should ever be writing. However, I'm not terribly motivated to do that patch myself because Clang isn't (yet? :) ) complaining about anything except the stuff I fixed in this PR.

@aardappel
Copy link
Copy Markdown
Collaborator

LGTM

@aardappel aardappel merged commit 6b271b7 into google:master Mar 23, 2020
@Quuxplusone Quuxplusone deleted the assign-operators branch March 24, 2020 00:37
antiagainst added a commit to antiagainst/iree that referenced this pull request May 16, 2020
antiagainst added a commit to antiagainst/iree that referenced this pull request May 16, 2020
LuckyRu pushed a commit to LuckyRu/flatbuffers that referenced this pull request Oct 2, 2020
…oogle#5829)

For example:

    include/flatbuffers/reflection.h:365:8: error: definition of implicit copy
          constructor for 'pointer_inside_vector<flatbuffers::Table, unsigned char>'
          is deprecated because it has a user-declared copy assignment operator
          [-Werror,-Wdeprecated-copy]
      void operator=(const pointer_inside_vector &piv);
           ^

It's unclear why the old code wanted to declare a public `operator=`
without defining it; that just seems like a misunderstanding of the C++03 idiom
for deleting a member function. And anyway, we don't *want* to delete the
assignment operator; these are polymorphic types that do not follow value
semantics and nobody should ever be trying to copy them. So the simplest fix
is just to go back to the Rule of Zero: remove the declaration of `operator=`
and let the compiler do what it wanted to do originally anyway.
"The best code is no code."

Also, update the generated .h files.

Fixes google#5649.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Clang 10]: definition of implicit copy constructor for 'TableKeyComparatoris deprecated

3 participants