RFR: 8338471: Refactor Method::get_new_method() for better NoSuchMethodError handling [v2]

[Coleen Phillimore]

On Mon, 9 Sep 2024 21:45:00 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Coleen is correct, all the callers do indeed check that the method `is_old()` but I think it's fine if we assert that in `get_new_method()` to make it clear that it is a requirement. The method `method_with_idnum()` can return nullptr here:
>> 
>>   if (m == nullptr || m->method_idnum() != idnum) {
>>     for (int index = 0; index < methods()->length(); ++index) {
>>       m = methods()->at(index);
>>       if (m->method_idnum() == idnum) {
>>         return m;
>>       }
>>     }
>>     // None found, return null for the caller to handle.
>>     return nullptr;
>> 
>> As the comment suggests, the caller should handle nullptr, which in this case is `get_new_method()`. The callers of get_new_method() try to handle this but I think it's cleaner to check inside the method.
>
> So is it the case that `nullptr` implies deleted, and deleted implies `nullptr`? If so checking for both is redundant and confusing because it makes it look like there are two distinct cases.

So that implies that you trust my reading of this code.  It is complicated enough that testing both seems like a safe thing to do and somewhat clarifying, or else adding an assert like:

    assert(new_method != nullptr || old_method->is_deleted(), "this is the only way this happens");
    return new_method == nullptr ? nsme : new_method;

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20874#discussion_r1751012295