On Mon, 4 Jan 2021 04:31:02 GMT, Hao Sun <github.com+16932759+shqk...@openjdk.org> wrote:
>>> _Mailing list message from [Kim Barrett](mailto:kim.barr...@oracle.com) on >>> [build-dev](mailto:build-dev@openjdk.java.net):_ >>> >>> > On Dec 24, 2020, at 3:44 PM, Xin Liu <xliu at openjdk.java.net> wrote: >>> > On Thu, 24 Dec 2020 17:27:39 GMT, Kim Barrett <kbarrett at openjdk.org> >>> > wrote: >>> > > [?] >>> > > Since DUIterator_Last is just delegating both the copy constructor and >>> > > assignment operator to the base class, their copy constructor and >>> > > assignment operator would be better as the default (either implicit or >>> > > explicit using `=default`) rather than explicit code. >>> > >>> > >>> > Agree. c2 actually never uses the assignment operator of >>> > DUIterator_Last. It needs the copy constructor in this line. >>> > https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/node.hpp#L1499 >>> > you can delete the following code or leave it =default. >>> > - void operator=(const DUIterator_Last& that) >>> > - { DUIterator_Fast::operator=(that); } >>> >>> DUIterator_Last::operator= *is* used, for example: >>> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/node.hpp#L1473 >>> >>> That doesn't change whether defaulting DUIIterator_Last's copy constructor >>> and copy assign works though (whether implicit or explicit). So making it >>> implicit does work. >>> >>> I think making it explicitly defaulted might be better though, to make it >>> clear the default behavior is intentional and it wasn't accidentally left as >>> the implicit default. This is because the default isn't right for the other >>> related classes. >> >> Yes. You're right. It's much better to make it explicitly defaulted. >> May I have your opinion @navyxliu ? > >> _Mailing list message from [Kim Barrett](mailto:kim.barr...@oracle.com) on >> [build-dev](mailto:build-dev@openjdk.java.net):_ >> >> > On Dec 29, 2020, at 10:33 PM, Hao Sun <github.com+16932759+shqking at >> > openjdk.java.net> wrote: >> > > _Mailing list message from [Kim Barrett](mailto:kim.barrett at >> > > oracle.com) on [build-dev](mailto:build-dev at openjdk.java.net):_ >> > > C++17 "guaranteed copy elision" is implemented in gcc7. >> > > http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0135r1.html >> > >> > >> > Thanks for your comment. >> > Initially we only suspected it might be copy elision that made gcc and >> > clang to behave differently on this warning, and we were not aware of this >> > flag '-fno-elide-constructors'. >> > Thank you for pointing it out. >> > > Using -fno-elide-constructors makes it obvious that the deprecated >> > > implicit copy constructors are indeed being elided, so no deprecation >> > > warning. >> > >> > >> > I suppose you want to express 'Using -**felide-constructors**' here. >> > gcc with '-fno-elide-constructos' would produce derepcated warnings for >> > this issue as clang-10 does. >> >> I really did mean "-fno-elide-constructors". The idea is that having the >> normally working build fail with "-fno-elide-constructors" provides evidence >> for the "working because of copy elision" conjecture. >> >> clang has supported -f[no-]elide-constructors for a while. >> >> It appears that either clang is different from gcc for -felide-constructors >> (which seems unlikely), or clang (clang-10) is doing the deprecation warning >> at a different point from gcc (quite plausible). That is, clang could be >> checking for the deprecated case before eliding the call, while gcc is >> checking for the deprecated case after copy elision has been applied. > > Thanks for your reply. > I checked the source code of clang-10 and gcc-9 and found that: > > 1) for clang-10, > 'Wdeprecated-copy' is implemented at the 'Sema' module of clang. See > https://github.com/llvm/llvm-project/blob/release/10.x/clang/lib/Sema/SemaDeclCXX.cpp#L13585 > > Flag 'felide-constructors' would enable 'felide_constructors' and flag > 'fno-elide-constructors' would enables 'fno_elide_constructors'. (See > https://github.com/llvm/llvm-project/blob/release/10.x/clang/include/clang/Driver/Options.td). > Then 'ElideConstructors' will be set (See > https://github.com/llvm/llvm-project/blob/release/10.x/clang/lib/Frontend/CompilerInvocation.cpp#L2863) > Finally, constructors might be elided in several spots in 'CodeGen' module. > See: > https://github.com/llvm/llvm-project/blob/release/10.x/clang/lib/CodeGen/CGStmt.cpp#L1094 > https://github.com/llvm/llvm-project/blob/release/10.x/clang/lib/CodeGen/CGExprCXX.cpp#L611 > https://github.com/llvm/llvm-project/blob/release/10.x/clang/lib/CodeGen/CGDecl.cpp#L1405 > > As far as I know, 'Sema' module is conducted to check semantics errors before > 'CodeGen' module. > That verified your conjecture, i.e. 'clang could be checking for the > derepcated case before eliding the call'. > > 2) for gcc-9, > 'felide-constructors' and 'Wdeprecated-copy' are implemented in a number of > spots in gcc. I currently didn't figure out their execution order clearly. > > But in one of the use points at function build_over_call(), > 'flag_elide_constructors' (See > https://github.com/gcc-mirror/gcc/blob/releases/gcc-9/gcc/cp/call.c#L8538) is > handled **before** 'warn_deprecated_copy' (See > https://github.com/gcc-mirror/gcc/blob/releases/gcc-9/gcc/cp/call.c#L8608 and > https://github.com/gcc-mirror/gcc/blob/releases/gcc-9/gcc/cp/call.c#L8679) > > Hope that my finding is helpful. Thanks. > > _Mailing list message from [Kim Barrett](mailto:kim.barr...@oracle.com) on > > [build-dev](mailto:build-dev@openjdk.java.net):_ > > > On Dec 24, 2020, at 3:44 PM, Xin Liu wrote: > > > On Thu, 24 Dec 2020 17:27:39 GMT, Kim Barrett wrote: > > > > [?] > > > > Since DUIterator_Last is just delegating both the copy constructor and > > > > assignment operator to the base class, their copy constructor and > > > > assignment operator would be better as the default (either implicit or > > > > explicit using `=default`) rather than explicit code. > > > > > > > > > Agree. c2 actually never uses the assignment operator of > > > DUIterator_Last. It needs the copy constructor in this line. > > > https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/node.hpp#L1499 > > > you can delete the following code or leave it =default. > > > > > > * void operator=(const DUIterator_Last& that) > > > * { DUIterator_Fast::operator=(that); } > > > > > > DUIterator_Last::operator= _is_ used, for example: > > https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/node.hpp#L1473 > > That doesn't change whether defaulting DUIIterator_Last's copy constructor > > and copy assign works though (whether implicit or explicit). So making it > > implicit does work. > > I think making it explicitly defaulted might be better though, to make it > > clear the default behavior is intentional and it wasn't accidentally left as > > the implicit default. This is because the default isn't right for the other > > related classes. > > Yes. You're right. It's much better to make it explicitly defaulted. > May I have your opinion @navyxliu ? Sorry, I overlooked that copy assignment case. Making it explicitly default is better to me too. no objection! ------------- PR: https://git.openjdk.java.net/jdk/pull/1874
