1515356 - implicitly-declared 'IPC::Principal::Principal(const IPC::Principal&)' is deprecated [-Werror=deprecated-copy]

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect, P3)

Description

[Sylvestre Ledru [:Sylvestre]]

In file included from /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/ipc/ipdl/_ipdlheaders/mozilla/dom/PContentChild.h:9,
                  from /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/ContentChild.h:15,
                  from /root/firefox-gcc-last/security/manager/ssl/nsKeygenHandlerContent.cpp:13,
                  from /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/security/manager/ssl/Unified_cpp_security_manager_ssl1.cpp:92:
 /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/ipc/ipdl/_ipdlheaders/mozilla/dom/PContent.h: In constructor 'mozilla::dom::BlobURLRegistrationData::BlobURLRegistrationData(const nsCString&, const IPCBlob&, const Principal&, const bool&)':
 /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/ipc/ipdl/_ipdlheaders/mozilla/dom/PContent.h:2716:26: error: implicitly-declared 'IPC::Principal::Principal(const IPC::Principal&)' is deprecated [-Werror=deprecated-copy]
  2716 |         revoked_(_revoked)

We have a bunch of others

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Here is how llvm/clang review to fix their C++ (building with gcc)
https://reviews.llvm.org/D52863 - LLVM cleanup
https://reviews.llvm.org/D52864 - Clang cleanup

Comment 2

[Nathan Froyd [:froydnj]]

I can't see what the warning is really complaining about, nor why the proposed fixes make any difference other than making the warning go away.  What is the actual point of the warning?  Something related to base classes?

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Honestly, I just did a brute force to see what kind of warnings it is finding (bug 1514781)
I didn't look if they are relevant or not.

Comment 5

[Andi [:andi]]

Just as Nathan said, if something that is not very obvious I'm missing I don't understand why is this useful in this context. Looking other the LLVM fixes these are only relevant when the copy constructor implementation is the same as a default implementation, but even so this doesn't bring something very useful to our code, maybe it's more obvious the initial intent of the developer having something 'default' and not 'custom 'implemented.
My take on this is that we should close it.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1515356 - Do not fail the build in case of warning with -Wdeprecated-copy r?froydnj

New warning with gcc 9

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1472d595d0a7
Do not fail the build in case of warning with -Wdeprecated-copy r=froydnj

Comment 8

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/1472d595d0a7

Comment 9

[(Away)]

(In reply to Nathan Froyd [:froydnj] from comment #2)

What is the actual point of the warning?

(In reply to Andi-Bogdan Postelnicu [:andi] from comment #5)

Just as Nathan said, if something that is not very obvious I'm missing I
don't understand why is this useful in this context.

IMO clang does a better job of explaining it: error: definition of implicit copy constructor for 'Principal' is deprecated because it has a user-declared copy assignment operator [-Werror,-Wdeprecated-copy]

In other words this sounds like a rule-of-three warning. If a class has a user-provided copy assignment, then it probably needs to have a matching user-provided copy constructor (or at least a = default to indicate that nothing special is needed, but you thought through it.)

With clang-10 this warning still shows up, despite the top-level -Wno-error, because some of the security/ moz.builds come along and add -Wextra later on the command line, which re-enables -Wdeprecated-copy.

In the case of this Principal class I think the fix is easy enough, and I don't see any other failures in tree, so I think we should just fix the class and then we don't need the -Wno-error.

Comment 10

[(Away)]

Would you mind testing whether we can re-enable the warning on gcc-9? IMO this warning is worth keeping around, and we've now fixed all the instances that I'm aware of (at least on clang).

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Already done!
We are green now

Comment 12

[(Away)]

(In reply to Sylvestre Ledru [:Sylvestre] from comment #11)

Already done!

I'm confused about what you mean because we still have https://hg.mozilla.org/mozilla-central/file/tip/build/moz.configure/warnings.configure#l176 in the tree.

We are green now

So to clarify, does that mean we can back out this bug?

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Nah, we still have occurrences:

/root/firefox-gcc-last/js/src/frontend/BinASTParserPerTokenizer.cpp:812:16:   required from here
/root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/js/Utility.h:497:33: error: implicitly-declared 'constexpr js::frontend::Directives::Directives(const js::frontend::Directives&)' is deprecated [-Werror=deprecated-copy]
  497 |     return MOZ_LIKELY(memory) ? new (memory) T(std::forward<Args>(args)...) \
      |                                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/root/firefox-gcc-last/js/src/ds/LifoAlloc.h:880:3: note: in expansion of macro 'JS_DECLARE_NEW_METHODS'
  880 |   JS_DECLARE_NEW_METHODS(new_, alloc, MOZ_ALWAYS_INLINE)
      |   ^~~~~~~~~~~~~~~~~~~~~~

In file included from /root/firefox-gcc-last/js/src/gc/Zone.h:22,
                 from /root/firefox-gcc-last/js/src/gc/Heap-inl.h:13,
                 from /root/firefox-gcc-last/js/src/gc/StoreBuffer-inl.h:15,
                 from /root/firefox-gcc-last/js/src/gc/StoreBuffer.cpp:7:
/root/firefox-gcc-last/js/src/vm/AtomsTable.h: In static member function 'static void js::AtomHasher::rekey(js::AtomStateEntry&, const js::AtomStateEntry&)':
/root/firefox-gcc-last/js/src/vm/AtomsTable.h:87:9: error: implicitly-declared 'constexpr js::AtomStateEntry& js::AtomStateEntry::operator=(const js::AtomStateEntry&)' is deprecated [-Werror=deprecated-copy]
   87 |     k = newKey;
      |         ^~~~~~

In file included from /root/firefox-gcc-last/js/src/vm/StringType.h:21,
                 from /root/firefox-gcc-last/js/src/jit/IonTypes.h:20,
                 from /root/firefox-gcc-last/js/src/wasm/WasmTypes.h:35,
                 from /root/firefox-gcc-last/js/src/wasm/WasmFrameIter.h:24,
                 from /root/firefox-gcc-last/js/src/wasm/WasmStubs.h:22,
                 from /root/firefox-gcc-last/js/src/wasm/WasmStubs.cpp:19,
                 from Unified_cpp_js_src_wasm3.cpp:2:
/root/firefox-gcc-last/js/src/gc/Barrier.h: In instantiation of 'js::WeakHeapPtr<T>& js::WeakHeapPtr<T>::operator=(const js::WeakHeapPtr<T>&) [with T = js::TaggedProto]':
/root/firefox-gcc-last/js/src/vm/Shape.h:1543:8:   required from here
/root/firefox-gcc-last/js/src/gc/Barrier.h:726:17: error: implicitly-declared 'constexpr js::TaggedProto& js::TaggedProto::operator=(const js::TaggedProto&)' is deprecated [-Werror=deprecated-copy]
  726 |     this->value = v.value;
      |     ~~~~~~~~~~~~^~~~~~~~~

etc

Comment 14

[Sylvestre Ledru [:Sylvestre]]

log:
https://relman-ci.moz.tools/job/firefox-gcc-9-last/728/