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

Reply via email to