RFR(XS): 8169734: Update uses of string "java.base" to macro
Daniel D. Daugherty
daniel.daugherty at oracle.com
Mon Dec 5 21:50:12 UTC 2016
A reply to Serguei's last comment below...
On 12/5/16 2:05 PM, Rachel Protacio wrote:
> Great, thank you Serguei! I'll commit.
>
> Rachel
>
>
> On 12/5/2016 4:03 PM, serguei.spitsyn at oracle.com wrote:
>> Hi Rachel,
>>
>>
>> It is good, thanks!
>>
>>
>> On 12/5/16 09:16, Daniel D. Daugherty wrote:
>>> On 12/5/16 9:41 AM, Rachel Protacio wrote:
>>>> Thanks, Dan! Ok, here's the webrev with all the instances changed.
>>>> http://cr.openjdk.java.net/~rprotacio/8169734.01/
>>>
>>> src/share/vm/classfile/classLoader.cpp
>>> No comments.
>>>
>>> src/share/vm/classfile/javaClasses.cpp
>>> No comments.
>>>
>>> src/share/vm/classfile/moduleEntry.cpp
>>> No comments.
>>>
>>> src/share/vm/classfile/modules.cpp
>>> No comments.
>>>
>>> src/share/vm/classfile/packageEntry.cpp
>>> No comments.
>>>
>>> src/share/vm/classfile/vmSymbols.hpp
>>> No comments.
>>>
>>> src/share/vm/oops/arrayKlass.cpp
>>> No comments.
>>>
>>> src/share/vm/oops/instanceKlass.cpp
>>> No comments.
>>>
>>> src/share/vm/runtime/arguments.cpp
>>> No comments.
>>>
>>> Thumbs up!
>>>
>>> I did not do a "completeness" check. I presume that Serguei will
>>> do that when he re-reviews.
>>
>> It seems complete to me.
>> There is one more instance of naked "java.base" though:
>> runtime/os.cpp: char* base_classes =
>> format_boot_path("%/modules/java.base", home, home_len, fileSep,
>> pathSep);
Doesn't seem that weird to me:
char* base_classes = format_boot_path("%/modules/java.base", home,
home_len, fileSep, pathSep);
changes to:
char* base_classes = format_boot_path("%/modules/" JAVA_BASE_NAME, home,
home_len, fileSep, pathSep);
which is normal C++ string concatenation...
Dan
>>
>> but it seems weird to use the macro in the path name.
>>
>>
>> Thanks,
>> Serguei
>>
>>>
>>> Dan
>>>
>>>
>>>>
>>>> Rachel
>>>>
>>>>
>>>> On 12/2/2016 4:18 PM, Daniel D. Daugherty wrote:
>>>>> > I'd suggest to collect at least one more opinion/review on this.
>>>>>
>>>>> Let's get this correct now and not have to come back and fix it down
>>>>> the road. Looks to me like these new locations can take advantage
>>>>> of C++ string concatenation unless I'm missing something, e.g.:
>>>>>
>>>>> 775 "Incorrect boot loader search path, no java
>>>>> runtime image or java.base exploded build");
>>>>>
>>>>> becomes:
>>>>>
>>>>> 775 "Incorrect boot loader search path, no java
>>>>> runtime image or " JAVA_BASE " exploded build");
>>>>>
>>>>> which is a bit long, but not too hard to deal with... And since we're
>>>>> migrating away from an informal limit of 80 chars per line... I won't
>>>>> even complain... :-)
>>>>>
>>>>> Dan
>>>>>
>>>>>
>>>>> On 12/2/16 1:52 PM, serguei.spitsyn at oracle.com wrote:
>>>>>> Hi Rachel,
>>>>>>
>>>>>>
>>>>>> On 12/2/16 11:12, Rachel Protacio wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Thanks for the review. Yeah, we had thought about that. Two
>>>>>>> concerns: (1) the concatenation or %s is a little verbose and
>>>>>>> possibly excessive for these ancillary error messages, and (2)
>>>>>>> because they are error messages, potentially someone could
>>>>>>> hard-code something to expect the output and if for some reason
>>>>>>> we change the macro in the future it could throw people off
>>>>>>> unnecessarily... Just some thoughts.
>>>>>>
>>>>>> Yes, I was thinking about the same.
>>>>>>
>>>>>>
>>>>>>> I'm happy to change all of them though if that's what we want.
>>>>>>
>>>>>> I'd suggest to collect at least one more opinion/review on this.
>>>>>> It is Ok with me to keep it as it is.
>>>>>>
>>>>>>
>>>>>>> I'll definitely change the classfile/vmSymbols.hpp and
>>>>>>> runtime/os.cpp instances.
>>>>>>
>>>>>> Thanks!
>>>>>>
>>>>>> Thanks,
>>>>>> Serguei
>>>>>>
>>>>>>
>>>>>>> Let me know what you think!
>>>>>>>
>>>>>>> Rachel
>>>>>>>
>>>>>>> On 12/1/2016 5:43 PM, serguei.spitsyn at oracle.com wrote:
>>>>>>>> Hi Rachel,
>>>>>>>>
>>>>>>>> It looks good in general.
>>>>>>>> However, there are more instances of java.base that may need an
>>>>>>>> update.
>>>>>>>>
>>>>>>>> classfile/classLoader.cpp:
>>>>>>>>
>>>>>>>> 775 "Incorrect boot loader search path, no java
>>>>>>>> runtime image or java.base exploded build");
>>>>>>>>
>>>>>>>> 1826 vm_exit_during_initialization("Unable to create
>>>>>>>> ModuleEntry for java.base");
>>>>>>>>
>>>>>>>>
>>>>>>>> classfile/modules.cpp:
>>>>>>>>
>>>>>>>> 180 "Bad package name for module: java.base");
>>>>>>>>
>>>>>>>> 185 err_msg("Invalid package name: %s for
>>>>>>>> module: java.base", package_name));
>>>>>>>>
>>>>>>>> 191 err_msg("Duplicate package name: %s for
>>>>>>>> module java.base",
>>>>>>>>
>>>>>>>> 209 assert(ModuleEntryTable::javabase_moduleEntry() != NULL,
>>>>>>>> "No ModuleEntry for java.base");
>>>>>>>>
>>>>>>>> 230 assert(pkg != NULL, "Unable to create a
>>>>>>>> java.base package entry");
>>>>>>>>
>>>>>>>> 244 "Module java.base is already defined");
>>>>>>>>
>>>>>>>> 253 log_debug(modules)("define_javabase_module(): Definition
>>>>>>>> of module: java.base,"
>>>>>>>>
>>>>>>>> 261 log_trace(modules)("define_javabase_module(): creation of
>>>>>>>> package %s for module java.base",
>>>>>>>>
>>>>>>>> 716 assert(ModuleEntryTable::javabase_defined(), "Attempt to
>>>>>>>> call get_module before java.base is defined");
>>>>>>>>
>>>>>>>> 762 "Attempt to call get_module_from_pkg before
>>>>>>>> java.base is defined");
>>>>>>>>
>>>>>>>> 799 "Attempt to call get_named_module before
>>>>>>>> java.base is defined");
>>>>>>>>
>>>>>>>>
>>>>>>>> runtime/arguments.cpp:
>>>>>>>>
>>>>>>>> 3432 vm_exit_during_initialization("Cannot specify
>>>>>>>> java.base more than once to --patch-module");
>>>>>>>>
>>>>>>>>
>>>>>>>> Some other files have it too:
>>>>>>>>
>>>>>>>> classfile/vmSymbols.hpp: template(java_base,
>>>>>>>> "java.base") \
>>>>>>>> classfile/moduleEntry.cpp: fatal("Unable to finalize module
>>>>>>>> definition for java.base");
>>>>>>>> classfile/moduleEntry.cpp: assert(jb_module != NULL,
>>>>>>>> "java.base ModuleEntry not defined");
>>>>>>>> classfile/moduleEntry.cpp: fatal("Unable to patch the module
>>>>>>>> field of classes loaded prior to java.base's definition,
>>>>>>>> invalid java.lang.reflect.Module");
>>>>>>>> classfile/packageEntry.cpp: vm_exit_during_initialization("A
>>>>>>>> non-java.base package was loaded prior to module system
>>>>>>>> initialization", entry->name()->as_C_string());
>>>>>>>> oops/arrayKlass.cpp: "module entry not available post
>>>>>>>> java.base definition");
>>>>>>>> oops/instanceKlass.cpp:
>>>>>>>> assert(ModuleEntryTable::javabase_moduleEntry() != NULL,
>>>>>>>> "java.base module is NULL");
>>>>>>>> runtime/os.cpp: char* base_classes =
>>>>>>>> format_boot_path("%/modules/java.base", home, home_len,
>>>>>>>> fileSep, pathSep);
>>>>>>>>
>>>>>>>> So, the question is if it's worth to update the remaining
>>>>>>>> instances as well.
>>>>>>>> Simple concatenation or %s replacement in some rare cases could
>>>>>>>> be used for it.
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Serguei
>>>>>>>>
>>>>>>>>
>>>>>>>> On 12/1/16 13:20, Rachel Protacio wrote:
>>>>>>>>> Hi!
>>>>>>>>>
>>>>>>>>> Please review this little change, replacing errant uses of the
>>>>>>>>> string "java.base" with the macro JAVA_BASE_NAME. Passes JPRT.
>>>>>>>>>
>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8169734
>>>>>>>>> Open webrev: http://cr.openjdk.java.net/~rprotacio/8169734.00/
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Rachel
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-runtime-dev
mailing list