On Wed, 6 Jan 2021 03:14:48 GMT, Hao Sun <github.com+16932759+shqk...@openjdk.org> wrote:
>> node.hpp changes seems fine. >> Passed tier1 builds and testing. > >> > I think the two issues described here are distinct and should be dealt >> > with in separate bugs and PRs. Their only relation is that both arise >> > with using clang-10. But they are very different problems, in very >> > different parts of the code, and probably ought to be reviewed by >> > folks from different teams. >> >> Thanks for your comment. >> >> Warning message of '-Wimplicit-int-float-conversion' was further encountered >> after we fixed the build failure caused by '-Wdeprecated-copy' first. That's >> why we put them in one PR initially. >> >> Yes. Your way is much better. But we suppose the issue of >> '-Wimplicit-int-float-conversion' is trivial and putting them in separate >> PRs might raise another internal review process (for our side) by which >> extra time is needed. I was wondering could we continue in one single PR. :) > > Will split this PR. > In this PR, we focus on the warnings caused by -Wdeprecated-copy. > Will update the code soon. Will create a new PR to address JDK-8259288. > 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 addressed. > > 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. (JDK-8259036) > 3. the implementation of 'operator=' for class DUIterator_Fast might be > problematic. @kimbarrett Regarding problem 1, I believe this is because there exists use-defined dtor for class DUIterator and the deprecated copy ctor warning message ought to be emitted by '-Wdeprecated-copy-dtor' (which is not enabled by '-Wall' or '-Wextra'). Please refer to https://github.com/llvm/llvm-project/blob/release/10.x/clang/lib/Sema/SemaDeclCXX.cpp#L13585 The main logic is user-defined dtor/copy constructor/assignment operator is checked in order when diagnosing implicit-defined copy constructor, and warning message will be emitted (lines 13619 to 13625). In this case of class DUIterator, user-defined dtor is provided but '-Wdeprecated-copy-dtor' is not enabled, clang-10 does not further check whether '-Wdeprecated-copy' is on or not. I further checked 1) I built openjdk with "--with-extra-cxxflags='-Wdeprecated-copy-dtor'", but the building failed earlier before class DUIterator. 2) I removed the dtor of class DUIterator manually and built openjdk with clang-10. Then clang-10 would produce the warning as we expected, which I think verified my conjecture. === Output from failing command(s) repeated here === * For target hotspot_variant-server_libjvm_gtest_objs_test_mathexact.o: In file included from /home/haosun01/jdk/jdk_src/test/hotspot/gtest/opto/test_mathexact.cpp:26: In file included from /home/haosun01/jdk/jdk_src/src/hotspot/share/opto/mulnode.hpp:28: void operator=(const DUIterator& that) ^ { return DUIterator(this, 0); } ^ void operator=(const DUIterator_Fast& that) ^ return DUIterator_Fast(this, 0); ^ ... (rest of output omitted) * For target support_gensrc_jdk.localedata__cldr-gensrc.marker: * All command lines available in /home/haosun01/tmp/deprecated-copy/clang10-fast-build/make-support/failure-logs. === End of repeated output === ------------- PR: https://git.openjdk.java.net/jdk/pull/1874
