On Thu, 23 Nov 2023 06:03:15 GMT, Thomas Stuefe <stu...@openjdk.org> 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