On Mon, 4 Jan 2021 19:33:45 GMT, Xin Liu <x...@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 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!
Thanks for your comments. @kimbarrett and @navyxliu I updated the patch based on my understanding. Please check the latest commit. As @kimbarrett mentioned, I suppose there still exist the following problems to be address. 1) why clang-10 with '-Wdeprecated-copy' does NOT raise warning for class DUIterator. It's weird. 2) the assert failure when building with gcc '-fno-elide-constructors'. Might not be related to our patch. 3) the implementation of 'operator=' for class DUIterator_Fast might be problematic. ------------- PR: https://git.openjdk.java.net/jdk/pull/1874
