On Mon, 25 Nov 2024 07:17:33 GMT, Emanuel Peter <epe...@openjdk.org> wrote:
>> Jatin Bhateja has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Testpoints for new value transforms + code cleanups
>
> src/hotspot/share/opto/connode.cpp line 49:
>
>> 47: switch( t->basic_type() ) {
>> 48: case T_INT: return new ConINode( t->is_int() );
>> 49: case T_SHORT: return new ConHNode( t->is_half_float_constant() );
>
> That will be quite confusing.... don't you think?
I mean do we need this? We already have `ConHNode::make` below...?
> src/hotspot/share/opto/divnode.cpp line 765:
>
>> 763: if((t1 == bot) || (t2 == bot) ||
>> 764: (t1 == Type::BOTTOM) || (t2 == Type::BOTTOM))
>> 765: return bot;
>
> Suggestion:
>
> if((t1 == bot) || (t2 == bot) ||
> (t1 == Type::BOTTOM) || (t2 == Type::BOTTOM)) {
> return bot;
> }
>
> Again: please always use brackets.
Apply the same below.
> src/hotspot/share/opto/divnode.cpp line 804:
>
>> 802:
>> 803:
>> //------------------------------Idealize---------------------------------------
>> 804: Node *DivHFNode::Ideal(PhaseGVN* phase, bool can_reshape) {
>
> Suggestion:
>
> Node* DivHFNode::Ideal(PhaseGVN* phase, bool can_reshape) {
Ok, and please add brackets for all the ifs below!
> src/hotspot/share/opto/divnode.cpp line 805:
>
>> 803:
>> //------------------------------Idealize---------------------------------------
>> 804: Node *DivHFNode::Ideal(PhaseGVN* phase, bool can_reshape) {
>> 805: if (in(0) && remove_dead_region(phase, can_reshape)) return this;
>
> Suggestion:
>
> if (in(0) != nullptr && remove_dead_region(phase, can_reshape)) { return
> this; }
>
> brackets for if and no implicit null checks please!
https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md
`Do not use ints or pointers as (implicit) booleans with &&, ||, if, while.
Instead, compare explicitly, i.e. if (x != 0) or if (ptr != nullptr), etc.`
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21490#discussion_r1855959810
PR Review Comment: https://git.openjdk.org/jdk/pull/21490#discussion_r1855985811
PR Review Comment: https://git.openjdk.org/jdk/pull/21490#discussion_r1855995743
PR Review Comment: https://git.openjdk.org/jdk/pull/21490#discussion_r1855999519
