RFR: 8258010: Debug build failure with clang-10 due to -Wdeprecated-copy and -Wimplicit-int-float-conversion [v2]

[Kim Barrett]

On Mon, 4 Jan 2021 06:59:54 GMT, Hao Sun <github.com+16932759+shqking at openjdk.org> wrote:

>> src/hotspot/share/opto/node.hpp line 1396:
>> 
>>> 1394: 
>>> 1395:   DUIterator_Fast(const DUIterator_Fast& that)
>>> 1396:     { _outp = that._outp;               debug_only(reset(that)); }
>> 
>> `reset` tests `_vdui`, but nothing here has set it, so it's uninitialized and that test wil be UB.
>> 
>> I'm also not sure why it's okay for `operator=` to use whatever the current value of `_vdui` might be; that could leave `_last` as the old value rather than refreshing from `that`, which seems wrong.  This is aabout pre-existing code that looks questionable to me.
>
> I suppose the constructor would be invoked before the copy assignment operator. That is `_vdui` gets initialized already in the ctor `DUIterator_Fast()` for `operator=` case. Right?
> Take the following code snippet as an example.
> DUIterator_Fast t1, t2;
> t2 = t1;    // assignment operator
> DUIterator_Fast t3 = t1;      // copy constructor. same as "t3(t1)"
> My point is that, the ctor for `t2` has already invoked, i.e. initializing `_vdui` as false. That's why `operator=` works well.
> 
> 
> Yes. For our newly-added copy constructor for `DUIterator_Fast`, we should initialize `_vdui` as `false`. It may be defined as below.
> DUIterator_Fast(const DUIterator_Fast& that)
>     { _outp = that._outp;               debug_only(_vdui = false; reset(that)); }

That's true on the first assignment of `t2`. But what if `t2` is reassigned
to some other iterator. That assignment sees `_vdui` true, and keeps the old
value of `_last` rather than updating the value from that other iterator. Is
that really correct? It certainly seems strange. I'm trying to find someone
who understands this code to get involved, but holidays.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1874