On Thu, 23 Nov 2023 06:03:15 GMT, Thomas Stuefe <[email protected]> wrote:
>> suchismith1993 has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> change macro position
>
> src/hotspot/os/aix/os_aix.cpp line 3065:
>
>> 3063: //Replaces provided path with alternate path for the given file,if it
>> doesnt exist.
>> 3064: //For AIX,this replaces .so with .a.
>> 3065: void os::Aix::mapAlternateName(char* buffer, const char *extension) {
>
> The documentation is wrong:
>
> // Replaces the specified path with an alternative path for the
> given file if the original path doesn't exist
>
> It does no such thing; it replaces the extension unconditionally. The comment
> sounds like it does a file system check.
>
> The whole function is not that well named - "map alternate name" does not
> really tell me anything, I need to look at the implementation and the caller
> to understand what it is doing. There is no mapping here, this is just a
> string utility function.
>
> The function should not modify the original buffer but instead assemble a
> copy. That is the conventional way to do these things. You can work with
> immutable strings as input, e.g. literals, and don't risk buffer overflows.
>
> All of this should be handled inside os_aix.cpp; see my other comment. This
> should not live in the external os::aix interface, since it has nothing to do
> with AIX. But I think all of this should be confined to os_aix.cpp.
>
> Proposal for a clearer name, comment, and pseudocode
>
> // Given a filename with an extension, return a new string containing the
> filename with the new extension.
> // New string is allocated in resource area.
> static char* replace_extension_in_filename(const char* filename, const char*
> new_extension) {
> - allocate buffer in RA
> - assemble new path by contacting old path - old extension + new extension
> - return new path
> }
thank you for the explanation. I am working on it.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16604#discussion_r1404265530
