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
PPCSymbolDB: Improve symbol map column detection #8932
Conversation
|
|
||
| // Two columns format: | ||
| // 80004000 zz_80004000_ | ||
| if (!(oss >> word) || word.length() != 8 || !is_hex_str(word)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you consider isxdigit coupled with std::all_of ? Not sure if there are any advantages to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did and I need to wrap isxdigit(int) around a lambda with an unsigned cast to avoid an assert error (char might be signed and negative) or use the locale version of it. However, I don't know if isxdigit is faster or not.
| // 80004000 zz_80004000_ | ||
| if (!(oss >> word) || word.length() != 8 || !is_hex_str(word)) | ||
| continue; | ||
| column_count = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be faster to tokenize the string by spaces to determine the column count, then later validate whether they are hex or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it will be faster since we don't need to tokenize the whole line if the first word isn't a hexstring. If I understand correctly it will also require a container like a vector to store the string tokens and then process them which might be slower.
|
@jordan-woyak Good catch, done. Rebased on latest master and I renamed |
|
Just a simple rebase. I've tested it against the map files I have and didn't find any regression. |
This PR fixes https://dolp.in/i11995.
Ready to be review & merged.