RFR: 8303755: Clean up around JavacElements.getAllMembers [v3]

[Pavel Rappo]

On Tue, 7 Mar 2023 19:26:00 GMT, Jonathan Gibbons <jjg at openjdk.org> wrote:

>> Would it be even more clear if we introduced an explicit if-else?
>> 
>>     int thatRank = types.rank(that.type);
>>     int thisRank = types.rank(this.type);
>>     if (thatRank < thisRank) {
>>         return true;
>>     } else if (thatRank == thisRank) {
>>         return that.getQualifiedName().compareTo(this.getQualifiedName()) < 0;
>>     }
>
> IMO, that's not an improvement.
> 
> Generally, style guides suggest to break before binary operators, so I'd suggest one of
> 
> int thatRank = types.rank(that.type);
> int thisRank = types.rank(this.type);
> return thatRank < thisRank
>     || (thatRank == thisRank
>         && that.getQualifiedName().compareTo(this.getQualifiedName()) < 0);
> 
> 
> But this is now down to a more subjective opinion.   Either way is OK.

Addressed in 140afc8.

>> If you are considering an unconditional null check here, then you might want to consider a dedicated EOF flag instead. Alternatively, I can remove the `assert`, but leave the comment; for the next reader.
>> 
>> It's probably the case that iteration elements are never `null`. If they were `null`, we would either notice false positive EOF or see NPEs thrown from predicates.
>
> You were the one to add the `assert` ;-) 
> I just made a statement that overall, the coding style in javac has been to use `Assert` instead of `assert`, at least in those cases where it is appropriate to make an assertion.
> Looking at the code in more detail, it's not clear to me that either an assertion or a comment (here) is required. If you want a comment, I'd suggest a more declarative comment on the class itself.

Addressed in 140afc8.

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

PR: https://git.openjdk.org/jdk/pull/12904