Skip to content
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.

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

Backup for process name and state #1368

Merged

Conversation

Jimmy-653
Copy link
Contributor

Retrieve process name and state from stat string if status map doesn't contain them already.
(I checked for Uid and Gid too, but it seems they are not contained in the string)

Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

I left a commit to fix the checkstyle issues CI was complaining about, and changed name and state not to conflict with the class variables. You need to rework the parsing logic, though. Filenames (and thus executables) can have spaces in the name.

@dbwiddis dbwiddis added the hacktoberfest-accepted Accepted during Hacktoberfest label Oct 16, 2020
@coveralls
Copy link

coveralls commented Oct 16, 2020

Coverage Status

Coverage remained the same at 87.975% when pulling 946967c on J-Jimmy:upstream/1364_backup_process_name_state into e23ebd3 on oshi:master.

@Jimmy-653
Copy link
Contributor Author

I can respect spaces in the name if I extract it like this.
However to get the state, I do need to split by whitespaces....There just is nothing else. (At least on my testsystem)

Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

Looking good. A few more requests and it'll be good to go!

int nameEnd = stat.indexOf(")");
if (Util.isBlank(status.get("Name")) && nameStart < nameEnd) {
// remove leading and trailing parentheses
String statName = stat.substring(nameStart, nameEnd + 1);
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be nameStart + 1 to exclude the leading parenthesis and nameEnd (which is exclusive) to exclude the ending parenthesis?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did output the original name from the status map and the name I extract here, the indexes are right as they are (on Ubuntu 20.04 LTS with Java 1.8_265)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see what you're doing. In this line you're capturing the full string with both parentheses, but then in the next line you're starting the substring from 1, and going to length-1 (which, since it's exclusive, has length-2 as last character), effectively removing the two parenthesis. If you just use start+1, end on this line, you don't need a second line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, thats still in because I had the string split at first, obviously can be put together. I just woke up so I could do it in half an hour.

Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

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

I can make these fixes and merge if you'd like, but wanted to at least give feedback why.

return;
}

int nameStart = stat.indexOf("(");
Copy link
Member

Choose a reason for hiding this comment

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

Scrutinzer says using the character '(' is faster than "(". I'm not sure it matters but why not? :)

}

int nameStart = stat.indexOf("(");
int nameEnd = stat.indexOf(")");
Copy link
Member

@dbwiddis dbwiddis Oct 17, 2020

Choose a reason for hiding this comment

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

But benchmarks show only a tiny difference so I don't care that much. Still, if you're making other changes, the character version will make the CI tests happy and be a few unnoticed nanoseconds faster.

int nameEnd = stat.indexOf(")");
if (Util.isBlank(status.get("Name")) && nameStart < nameEnd) {
// remove leading and trailing parentheses
String statName = stat.substring(nameStart, nameEnd + 1);
Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see what you're doing. In this line you're capturing the full string with both parentheses, but then in the next line you're starting the substring from 1, and going to length-1 (which, since it's exclusive, has length-2 as last character), effectively removing the two parenthesis. If you just use start+1, end on this line, you don't need a second line.

@dbwiddis
Copy link
Member

Looks great! Merging.

@dbwiddis dbwiddis merged commit a4215a9 into oshi:master Oct 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Accepted during Hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants