-
Notifications
You must be signed in to change notification settings - Fork 36
Removed build_interpreter from main() #292
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
Removed build_interpreter from main() #292
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #292 +/- ##
==========================================
- Coverage 82.46% 82.00% -0.47%
==========================================
Files 20 20
Lines 958 950 -8
Branches 88 87 -1
==========================================
- Hits 790 779 -11
- Misses 168 171 +3
... and 1 file with indirect coverage changes
🚀 New features to boost your workflow:
|
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.
clang-tidy made some suggestions
e71a7eb
to
cce9ca4
Compare
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.
As I was going through the creation of the interpreter, i realized we anyways skip the first argument anways
Line 112 in 1158ecd
createInterpreter(Args(argv ? argv + 1 : argv, argv + argc)); |
So having 1 arg in the container is equally good as nothing.
This makes build_interpreter redundant as we just make some change on the 0th element through it !
clang-tidy review says "All clean, LGTM! 👍" |
Description
In the main function, call to
build_interpreter
is redundant. Instead calling directly toxcpp::interpreter::interpreter
will work fine.Reasoning
This sets first arg and then passes all the args to
xcpp::interpreter::interpreter
. Butxcpp::interpreter::interpreter
always skips the first arg(if the argv is non-null).But, what if the argv is
nullptr
??. It can happen if the user manually removes all the args fromkernel.json
(including the program name).It can be show that if
we are not passing any argument
is same aswe are passing only one argument
(i.e. the pgroam name).createInterpreter(Args(nullptr, nullptr));
is same ascreateInterpreter(Args(argv + 1, argv + 1));
.Proof