[cmpcodesize][2] Begin extracting regular expressions #638
Conversation
The functions in `cmpcodesize.compare` do several things: they call otool, they use regex matchers to extract information from otool's output, and they output that information using `print()`. Currently, the only way to test cmpcodesize is via functional tests: ones that actually run on a dylib like libswiftCore.dylib. This takes a lot of time and is difficult to fully automate. By extracting otool calls from `cmpcodesize.compare`, we can test those in isolation. Furthermore, future commits can test the functions in `cmpcodesize.compare` using canned strings, instead of actual otool output.
|
|
||
|
|
||
| # Cache the compiled regex into a global object. | ||
| _ARCHITECTURE_REGEX = re.compile('architecture ([\S]+)') |
There was a problem hiding this comment.
This could be \S+ without square brackets.
There was a problem hiding this comment.
Indeed it can, thanks for pointing that out! Just changed it and the unit tests still all pass. feelsgoodman. 😄
`cmpcodesize.compare` has a few responsibilities: matching regular expressions on otool output, and storing the results of those matches in dictionaries. Begin extracting the regular expression matching into a separate file, `cmpcodesize/regex.py`. This makes the code more modular, and allows for finer-grained unit tests.
modocache
force-pushed
the
cmpcodesize-regex
branch
from
0905208 to
b5cfd0a
Compare
|
This should probably be reviewed by @eeckstein , @swiftix, and @nadavrot. 👋 |
|
@modocache What are your plans for cmpcodesize? Are you planning to add ELF support? Are you planning to add additional diagnostics? |
|
I was initially planning on doing some code cleanup and improving test coverage, then discussing further with you folks! In discussions in #548 and #553 you mentioned CSV output. I was thinking about bringing that up on the mailing list to flesh out exactly what you were looking for. If there's anything else you're looking to do, I'd be happy to help! 👍 |
|
I created SR-407 to track supporting ELF in |
|
@modocache Are you planning to work on making the output CSV? It is annoying to manually copy-and-paste values from the current format. |
Sure! I'll create a JIRA issue when I get in front of a computer. |
|
I talked with @eeckstein and he said he would look at this. Assigning to him. |
|
@modocache Sorry for the long delay. Do you want to continue with this PR? |
|
@eeckstein Yeah I'd love to, assuming this script is still used. Is it? |
|
Sorry for letting this languish. I don't think I have the capacity to shepherd this at the moment. I'll close this pull request to reduce the amount of noise on the repository. Thanks! I also finally got around to creating a JIRA issue for outputting CSV: https://bugs.swift.org/browse/SR-2595. |
Update string processing tag to dev/8.
This pull request should be reviewed after #636 is merged. Sorry for any confusion. I split up the pull requests to make code review easier.
What's in this pull request?
cmpcodesize.comparehas a few responsibilities: matching regular expressions on otool output, and storing the results of those matches in dictionaries.This pull request begins extracting the regular expression matching into a separate file,
cmpcodesize/regex.py.Why merge this pull request?
What downsides are there to merging this pull request?
The migration process is not completely done. You'll notice a
FIXMEfor the other regex's still incompare.py. I'd like to address those in future pull requests, but the maintainers may prefer to do it all in one pull request.