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

handle parens in process names in b2g-info #130

Merged
merged 1 commit into from Nov 15, 2013
Merged

handle parens in process names in b2g-info #130

merged 1 commit into from Nov 15, 2013

Conversation

@lloyd
Copy link
Contributor

@lloyd lloyd commented Nov 13, 2013

would fix issue #129

@lloyd
Copy link
Contributor Author

@lloyd lloyd commented Nov 13, 2013

the regex fragment "(\\(.*\\)) [A-Z]"

will match ((Nuwa)) S

the scanf argument %17[^)] %d will not

@lloyd
Copy link
Contributor Author

@lloyd lloyd commented Nov 13, 2013

here's a sample program and tests you can confirm with: https://gist.github.com/lloyd/7452319

Seems like we have three options for parsing these lines:

  1. manually teeny lexer
  2. scanf
  3. regex

#2 is insufficiently expressive to parse the grammar. #1 is more work than it's worth (but I do love writing parsers), so that leaves us with #3.

and given this is a command line tool, I think the efficiency arguments w.r.t parsing are unimportant.

@lloyd
Copy link
Contributor Author

@lloyd lloyd commented Nov 13, 2013

(but w.r.t efficiency, I can get you before/after numbers for scanf regex if you like, convinced the delta won't even register, but am often wrong - at least that's what my wife tells me)

@dhylands
Copy link
Contributor

@dhylands dhylands commented Nov 13, 2013

Thanks for the test program. It's obviously using a greedy .* which is why it works.

This particular program (b2g-info) was created because getting the same information using sh was taking too long (IIRC its used by the get memory script). So there is a performance consideration, but the differences between the 2 implementation, as you suggest, may be inconsequential.

@lloyd
Copy link
Contributor Author

@lloyd lloyd commented Nov 14, 2013

(still have to actually build and test that patch, will do so in several hours and can squash into a single commit for clarity, please hold on merge)

@lloyd
Copy link
Contributor Author

@lloyd lloyd commented Nov 14, 2013

@dhylands In process.cpp nullptr is undefined: Would you like me to figure out where in the build system I must add -std=c++0x in order to get nullptr defined, or is there an mozillay include file?

@lloyd
Copy link
Contributor Author

@lloyd lloyd commented Nov 14, 2013

All nits addressed except for NULL vs. nullptr. I'm uncomfortable making significant build system changes and there's no precedent for using nullptr at this level that I can find (standalone command line system tools).

Ready for final review and merge?

@lloyd lloyd mentioned this pull request Nov 14, 2013
@dhylands
Copy link
Contributor

@dhylands dhylands commented Nov 15, 2013

I'll say leave it as NULL then. We're outside of the gecko code, and its a pretty minor difference.

I'm just wrapping up for the day, so I'll take a look at this tomorrow morning and merge.

Good catch on the memory leak.

dhylands added a commit that referenced this pull request Nov 15, 2013
handle parens in process names in b2g-info. r=dhylands
@dhylands dhylands merged commit 274f77b into mozilla-b2g:master Nov 15, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants