RFR(XS): 8169734: Update uses of string "java.base" to macro
Rachel Protacio
rachel.protacio at oracle.com
Tue Dec 6 19:14:08 UTC 2016
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