RFR(XS): 8169734: Update uses of string "java.base" to macro
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Mon Dec 5 21:55:19 UTC 2016
On 12/5/16 13:50, Daniel D. Daugherty wrote:
> 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:
I admit that I used wrong term ("weird") here. :)
It looks a little bit unusual 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...
I have no objection to fix it this way.
Thank you for commenting it.
Thanks,
Serguei
>
> 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