RFR: 8259839 SystemDictionary exports too much implementation [v2]

[Coleen Phillimore]

On Wed, 27 Jan 2021 05:00:41 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   return NULL for find_or_define_helper if pending exception
>
> src/hotspot/share/classfile/systemDictionary.cpp line 241:
> 
>> 239: static Klass* handle_resolution_exception(Symbol* class_name, bool throw_error, Klass* klass, TRAPS) {
>> 240:   if (HAS_PENDING_EXCEPTION) {
>> 241:     assert(klass == NULL, "Should not have result with exception pending");
> 
> If klass is expected to be NULL why even bother passing it in to this function?

It might not be NULL if there wasn't a pending exception (or failure to find the class).

> src/hotspot/share/classfile/systemDictionary.cpp line 240:
> 
>> 238: 
>> 239: static Klass* handle_resolution_exception(Symbol* class_name, bool throw_error, Klass* klass, TRAPS) {
>> 240:   if (HAS_PENDING_EXCEPTION) {
> 
> To me it would be clearer if the caller checked HAS_PENDING_EXCEPTION and only called handle_resolution_exception if there was one. As there are only a couple of calls this doesn't result in much code duplication.

handle_resolution_exception has two cases. 1. pending exception and 2. if klass is null.  It's really a check for both of those cases.  Maybe the refactoring doesn't help reduce the special case noise in these two callers.  I can put back the if statements before it.

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

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