On Wed, 30 Dec 2020 03:31:38 GMT, Hao Sun <github.com+16932759+shqk...@openjdk.org> wrote:
>> LGTM. It still needs other's approval. > >> _Mailing list message from [Kim Barrett](mailto:kim.barr...@oracle.com) on >> [build-dev](mailto:build-dev@openjdk.java.net):_ >> >> > On Dec 22, 2020, at 8:52 PM, Hao Sun <github.com+16932759+shqking at >> > openjdk.java.net> wrote: >> > 1. '-Wdeprecated-copy' >> > As specified in C++11 [1], "the generation of the implicitly-defined >> > copy constructor is deprecated if T has a user-defined destructor or >> > user-defined copy assignment operator". The rationale behind is the >> > well-known Rule of Three [2]. >> > Introduced since gcc-9 [3] and clang-10 [4], flag '-Wdeprecated-copy' >> > warns about the C++11 deprecation of implicitly declared copy >> > constructor and assignment operator if one of them is user-provided. >> > Defining an explicit copy constructor would suppress this warning. >> > The main reason why debug build with gcc-9 or higher succeeds lies in >> > the inconsistent warning behaviors between gcc and clang. See the >> > reduced code example [5]. We suspect it might be return value >> > optimization/copy elision [6] that drives gcc not to declare implicit >> > copy constructor for this case. >> >> 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. > >> Why doesn't this patch similarly define DUIterator copy constructor? >> It seems to have the same issue, and I'm surprised clang-10 didn't >> complain about it too. Certainly gcc with -fno-elide-constructors >> complains about the defaulted implicit constructor. >> > > I'm afraid we have noticed the same issue for 'DUIterator' before. > Yes. It's weird that clang-10 didn't raise warning here. ( verified it on my > local machine.) > >> But I discovered something alarming while experimenting. Building >> with gcc10.2 with -fno-elide-constructors doesn't seem to be possible. >> I get different kinds of failures depending on how DUIterator is >> defined: >> >> - implict: deprecation warning (as expected) >> - =delete: error, deleted function used >> - =default: assert in os::free >> - _idx and reset from that: assert in reset >> >> Without -fno-elide-constructors, all of the variants seem to work >> except =delete, which still fails because the deleted function is >> used. (I didn't test the "working" cases extensively though.) >> >> So there's something problematic, though I don't understand the code >> well enough to understand what. > > Thanks for your tests. > But I have no idea how to fix it right now either. > Do you know anyone who is familiar with these code and maybe we can invite > him/her to help take a look at this issue? > Thanks. > >> >> Interestingly, some of the complexity and weirdness around copy/assign >> for these classes may be an attempt at providing something like move >> semantics in a C++98 code base. I've noticed a number of places in >> HotSpot that are doing that. >> >> Defining DUIterator_Fast and DUIterator_Last as movable but not >> copyable (explicitly delete the copy constructor and copy assign >> operator, and define the move constructor and move assign operator >> with the reset) works, even with -fno-elide-constructors. > _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 ? ------------- PR: https://git.openjdk.java.net/jdk/pull/1874
