Skip to content
This repository was archived by the owner on Mar 8, 2020. It is now read-only.

Fix leaks in HasPosition calls - #102

Merged
juanjux merged 1 commit into
bblfsh:masterfrom
juanjux:fix/positions_leak
May 31, 2018
Merged

juanjux merged 1 commit into
bblfsh:masterfrom
juanjux:fix/positions_leak

Conversation

@juanjux

@juanjux juanjux commented May 31, 2018 •

Copy link
Copy Markdown
Contributor

Fixes #100.

  • Fix calls in the "HasXXX" position functions to AttributeValue without decreasing the reference of the returned object or adding it to the memory tracker which were leaking in a specific case

  • Added ugly warnings about the need to do that on Attribute and AttributeValue.

  • Added two new tests for memleaks covering the bug.

  • Updated the build to the latest libuast and changed URL.

Signed-off-by: Juanjo Alvarez [email protected]

@juanjux juanjux added the bug label May 31, 2018
@juanjux juanjux self-assigned this May 31, 2018
@juanjux
juanjux requested a review from dennwc May 31, 2018 16:53
@dennwc

dennwc commented May 31, 2018

Copy link
Copy Markdown
Member

This is exactly the reason why Go does not allow type conversion of ints to bool.

@dennwc

dennwc commented May 31, 2018

Copy link
Copy Markdown
Member

And please squash commits before merging.

@juanjux

juanjux commented May 31, 2018

Copy link
Copy Markdown
Contributor Author

@dennwc not arguing that is a good feature to have, but the problem was not calling decref on the value returned by Attribute (the reason for the boolean wrapper is that I also dislike these type conversions).

Signed-off-by: Juanjo Alvarez <[email protected]>

Remove debug stuff

Signed-off-by: Juanjo Alvarez <[email protected]>
@juanjux
juanjux force-pushed the fix/positions_leak branch from 59563c1 to 1ca9bec Compare May 31, 2018 17:35
@juanjux
juanjux merged commit d363170 into bblfsh:master May 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants