RFR: 8345911: Enhance error message when IncompatibleClassChangeError is thrown for sealed class loading failures [v4]

[David Holmes]

On Mon, 16 Dec 2024 22:34:43 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> David Holmes has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Update messages per feedback from Alan
>
> src/hotspot/share/classfile/moduleEntry.hpp line 191:
> 
>> 189:   const char* name_as_C_string() {
>> 190:     return is_named() ? name()->as_C_string() : UNNAMED_MODULE;
>> 191:   }
> 
> Maybe PackageEntry::name_as_C_string() should get the same helper function.

Can't do that. The un-named package means you have no PackageEntry and package() returns null.

> src/hotspot/share/oops/instanceKlass.cpp line 234:
> 
>> 232:              "and sealed class %s is in module '%s' with loader %s",
>> 233:              k->external_name(), k->module()->name_as_C_string(), k->module()->loader_data()->loader_name_and_id(),
>> 234:              this->external_name(), this->module()->name_as_C_string(), this->module()->loader_data()->loader_name_and_id());
> 
> I was going to suggest less verbosity by creating local variables above like this_loader_data and k_loader_data but maybe it's not that useful.  At any rate, could you put each of these long parameters on each line?

Done

> src/hotspot/share/oops/instanceKlass.cpp line 246:
> 
>> 244:              this->external_name(), this->package() != nullptr ? this->package()->name()->as_C_string() : "unnamed",
>> 245:              this->module()->loader_data()->loader_name_and_id());
>> 246:     log_trace(class, sealed)(" - %s", ss.as_string());
> 
> Same here - put each parameter on its own line.

Done

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

PR Review Comment: https://git.openjdk.org/jdk/pull/22703#discussion_r1887861438
PR Review Comment: https://git.openjdk.org/jdk/pull/22703#discussion_r1887861118
PR Review Comment: https://git.openjdk.org/jdk/pull/22703#discussion_r1887861633