RFR(XS): 8169734: Update uses of string "java.base" to macro
Daniel D. Daugherty
daniel.daugherty at oracle.com
Tue Dec 6 19:22:41 UTC 2016
A low priority JDK10 bug is fine by me. Just want it to be clean...
Thanks!
Dan
On 12/6/16 12:14 PM, Rachel Protacio wrote:
> Hi! Thanks for your comments; unfortunately, I had committed before I
> got these replies... If you'd like me to address this in a bug, please
> let me know.
> Rachel
>
> On 12/5/2016 4:55 PM, serguei.spitsyn at oracle.com wrote:
>> 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