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

Add tool to find directly recursive calls in toxcore. #1138

Merged
merged 1 commit into from
Sep 8, 2018

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Aug 27, 2018

We should avoid recursion, as it makes reasoning about stack growth
harder. This tool shows (currently) 2 (non-tail) recursive functions. We exempt only these. New recursive functions are not allowed.


This change is Reviewable

@iphydf iphydf added this to the v0.2.x milestone Aug 27, 2018
@nurupo
Copy link
Member

nurupo commented Sep 6, 2018

Does it still fail? You removed its use on Travis.

Copy link
Member

@nurupo nurupo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 1 approvals obtained (waiting on @iphydf)


other/analysis/check-recursion, line 19 at r1 (raw file):

            "opt -analyze -print-callgraph "
            "2>&1".format(inputs=INPUTS, cflags=CFLAGS),
            shell=True).split("\n")

Make that line

 shell=True).decode('utf8').split("\n")

That works for both Python 2 and Python 3.

Otherwise it fails in Python 3 with:

c-toxcore$ python3 t.py
[+0000=0000] Generating callgraph
Traceback (most recent call last):
  File "t.py", line 71, in <module>
    find_recursion()
  File "t.py", line 52, in find_recursion
    cg = load_callgraph()
  File "t.py", line 19, in load_callgraph
    shell=True).split("\n")
TypeError: a bytes-like object is required, not 'str'

Copy link
Member

@nurupo nurupo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codecov
Copy link

codecov bot commented Sep 6, 2018

Codecov Report

Merging #1138 into master will increase coverage by <.1%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1138     +/-   ##
========================================
+ Coverage    82.7%   82.8%   +<.1%     
========================================
  Files          82      82             
  Lines       14667   14667             
========================================
+ Hits        12140   12147      +7     
+ Misses       2527    2520      -7
Impacted Files Coverage Δ
toxcore/group.c 76% <0%> (-0.4%) ⬇️
toxcore/friend_connection.c 94.5% <0%> (-0.3%) ⬇️
toxcore/DHT.c 76.6% <0%> (-0.1%) ⬇️
toxcore/Messenger.c 86.5% <0%> (-0.1%) ⬇️
toxav/toxav.c 68% <0%> (+0.3%) ⬆️
toxav/msi.c 65.3% <0%> (+0.8%) ⬆️
toxcore/LAN_discovery.c 85.8% <0%> (+2.8%) ⬆️
auto_tests/toxav_many_test.c 96.6% <0%> (+4%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab4477d...93cc178. Read the comment docs.

Copy link
Member Author

@iphydf iphydf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iphydf iphydf force-pushed the check-recursion branch 9 times, most recently from f6c7a3a to b8c789c Compare September 6, 2018 17:04
Copy link
Member

@nurupo nurupo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 5 of 5 files at r2, 2 of 2 files at r3.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @iphydf)


other/analysis/check_recursion, line 81 at r3 (raw file):

            "-Itoxencryptsave",
            "$CPPFLAGS",
        )),

Seems like you forgot to remove inputs and cflags as they are not being used anywhere.

@iphydf iphydf force-pushed the check-recursion branch 3 times, most recently from dcf18b7 to a63f552 Compare September 6, 2018 18:07
Copy link
Member Author

@iphydf iphydf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @nurupo)


other/analysis/check_recursion, line 81 at r3 (raw file):

Previously, nurupo wrote…

Seems like you forgot to remove inputs and cflags as they are not being used anywhere.

Done.

@iphydf iphydf force-pushed the check-recursion branch 2 times, most recently from abc8a19 to 1bdce17 Compare September 8, 2018 11:14
Copy link
Member

@nurupo nurupo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r4.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @iphydf)


other/analysis/check_recursion, line 104 at r4 (raw file):

    "add_to_closest -> add_to_closest",
    "add_to_list -> add_to_list",
})

Funny how you went out of your way to make the script generic, to remove all of the hardcoded toxcore and clang commands, yet kept this expected in it. I'd expect the expected check to be done in another python file that imports this one just to call find_recursion(expected={...}). Well, fair enough, I assume you wanted to make the scrips as generic as possible, yet didn't want to make this a multi-file ordeal, so you have settled on having this non-generic part which can be easily removed if needed be.

We should avoid recursion, as it makes reasoning about stack growth
harder. This tool shows (currently) 4 (non-tail) recursive functions, at
least 2 of which are easy to fix.
@iphydf iphydf merged commit 93cc178 into TokTok:master Sep 8, 2018
@iphydf iphydf deleted the check-recursion branch September 8, 2018 22:21
@robinlinden robinlinden modified the milestones: v0.2.x, v0.2.8 Oct 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants