Skip to content
This repository has been archived by the owner on Jan 13, 2022. It is now read-only.

Dealing with Unity dsyms and integration tests #26

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Dealing with Unity dsyms and integration tests #26

wants to merge 1 commit into from

Conversation

phuesler
Copy link

  • Always read the name from the symbol table
  • Always use the first symbol if there are several symbols with the same address
  • Add integration tests
  • The integration tests show a bug with the cache. Without the cache disabled, they fail. With cache enabled they fail on first run and success on the 2nd run

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@zlandau
Copy link
Contributor

zlandau commented Jan 30, 2015

Thanks a lot! Conceptually these changes look good. Can you clean them up a bit and then I'll do a more thorough review?

@phuesler
Copy link
Author

phuesler commented Feb 3, 2015

I have cleaned up the code. As fare as I can tell, the commented out code was only in a specific commit and not in the final diff of the pull request. I also refactored the tests with the intent to clarify what they are actually testing. The last test that I added shows, that on empty cache but with caching activated, the first run does not return a symbol but the second one does.

@zlandau
Copy link
Contributor

zlandau commented Feb 11, 2015

Sorry for taking so long to get back to you, I will try to look over these today.

@zlandau
Copy link
Contributor

zlandau commented Feb 11, 2015

Ok mostly just style things, otherwise it seems ok. Normally I'd have you create a clean set of diffs, it's still pretty hard to review with things like commented out code that get removed later and lots of whitespace changes. But it's a pretty big stack of diffs so if you don't want to go back and clean it up I'm ok with it for this round.

The integration tests are really handy, I'll try to add some more at some point.

Thanks for your hard work!

@phuesler
Copy link
Author

Let's do this properly. I will redo the pull request so that the diff is easier to read and I'll clean up the formatting as well. I should have something ready by the end of next week. Feel free do close this pull request.

@zlandau
Copy link
Contributor

zlandau commented Feb 12, 2015

Ok great, thanks. You can just push it to the same pull request branch if you want.

* Always read the name from the symbol table
* Always use the first symbol if there are several symbols with the same
* address
* Add integration tests
* The integration tests show a bug with the cache. With cache disabled, they fail. With cache enabled they fail on first run and succeed on the 2nd run
@phuesler
Copy link
Author

I redid the pull request and squashed everything into one commit.

Do you have any pointers why atosl returns different results with a disabled cache?

@ghost
Copy link

ghost commented Aug 4, 2015

Thank you for reporting this issue and appreciate your patience. We've notified the core team for an update on this issue. We're looking for a response within the next 30 days or the issue may be closed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants