On Mon, 3 Apr 2023 12:59:12 GMT, Markus Grönlund <mgron...@openjdk.org> wrote:

>> src/hotspot/share/prims/agentList.cpp line 204:
>> 
>>> 202: 
>>> 203: // Invokes Agent_OnAttach for agents loaded dynamically during runtime.
>>> 204: jint AgentList::load_agent(const char* agent_name, const char* 
>>> absParam,
>> 
>> I feel that it is better to keep the original function name 
>> "load_agent_library". As you listed there two kinds of agents: Java and 
>> Native. The function name give a hint it is native agent. Also, it is better 
>> to avoid changes that aren't really necessary.
>
> I changed the names because I found it very hard to understand what the old 
> names represented: "AgentLibrary" vs "Library"? "add_init_agent" vs 
> "add_instrumentation_agent", or even "add_loaded_agent"? Also a bit confusing 
> that "load_agent_library" would also include statically linked agents - no 
> library is loaded there.

Okay.
Refactoring is usually not easy to review.
With a renaming it becomes harder, so it is better to be conservative.

There are other side effects to consider:

  - back porting also becomes harder
  - developers have to learn new names instead of already known
  
The good side is that your refactoring consolidates this code in a well known 
locations. :-)

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/12923#discussion_r1158094815

Reply via email to