Skip to content

8374232: Comment cleanup in diagnosticCommand.cpp #28962

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kevinjwalls wants to merge 2 commits into from
+2 −5
Closed

8374232: Comment cleanup in diagnosticCommand.cpp #28962

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d2f0eff
8374232: Comment cleanup in diagnosticCommand.cpp
kevinjwalls Dec 23, 2025
7d2bab9
update
kevinjwalls Dec 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions src/hotspot/share/services/diagnosticCommand.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,11 +91,10 @@ static void loadAgentModule(TRAPS) {
THREAD);
}

void DCmd::register_dcmds(){
// Registration of the diagnostic commands
// First argument specifies which interfaces will export the command
// Second argument specifies if the command is enabled
// Third argument specifies if the command is hidden
void DCmd::register_dcmds() {
// Registration of the diagnostic commands.
// Argument specifies to which interfaces a command is made available,
// can also specify if hidden from jcmd help.
Copy link
Contributor

@plummercj plummercj Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind a liked the "first argument, second argument" approach a bit more.

// First argument specifies on which interfaces a command is made available.
// Optional second argument specifies if hidden from jcmd help. Default is false.

Copy link
Contributor Author

@kevinjwalls kevinjwalls Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK thanks.
I was thinking as all the usages here only use one argument, the numbering seems irrelevant.
The flags/mask of sources is maybe unusual enough that it's worth keeping the comment.

I almost didn't even mention the hidden flag, you go and find the constructor DCmdFactoryImpl(uint32_t flags, bool hidden = false) when clarification is needed.
Any thoughts on if it's worth mentioning?

Copy link
Contributor

@plummercj plummercj Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, before adding my comment I had to first go digging for that bool hidden = false reference. At first it was unclear to me what the comment was even trying to describe. I think if you wanted to delete the comment that would be ok. I don't think we have a comment like this is any of the other numerous sections that register dcmds.

Copy link
Contributor Author

@kevinjwalls kevinjwalls Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, so removing the obscure hidden flag from the comment, it can just be:

void DCmd::register_dcmds() {
// Registration of the diagnostic commands.
// Argument specifies on which interfaces a command is made available.

But as it is called register_dcmds, commenting that we do "Registration of the diagnostic commands." on the next line is obvious/unnecessary.

Updated!

uint32_t full_export = DCmd_Source_Internal | DCmd_Source_AttachAPI
| DCmd_Source_MBean;
DCmdFactory::register_DCmdFactory(new DCmdFactoryImpl<HelpDCmd>(full_export));
Expand Down