Skip to content

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

Merged
merged 3 commits into from
May 1, 2025

Conversation

kr-2003
Copy link
Contributor

@kr-2003 kr-2003 commented Apr 27, 2025

Description

In the main function, call to build_interpreter is redundant. Instead calling directly to xcpp::interpreter::interpreter will work fine.

Reasoning

interpreter_ptr build_interpreter(int argc, char** argv)
{
    std::vector<const char*> interpreter_argv(argc);
    interpreter_argv[0] = "xeus-cpp";

    // Copy all arguments in the new vector excepting the process name.
    for (int i = 1; i < argc; i++)
    {
        interpreter_argv[i] = argv[i];
    }

    interpreter_ptr interp_ptr = std::make_unique<interpreter>(argc, interpreter_argv.data());
    return interp_ptr;
}

This sets first arg and then passes all the args to xcpp::interpreter::interpreter. But xcpp::interpreter::interpreter always skips the first arg(if the argv is non-null).

interpreter::interpreter(int argc, const char* const* argv) :
    xmagics()
    , p_cout_strbuf(nullptr)
    , p_cerr_strbuf(nullptr)
    , m_cout_buffer(std::bind(&interpreter::publish_stdout, this, _1))
    , m_cerr_buffer(std::bind(&interpreter::publish_stderr, this, _1))
{
    //NOLINTNEXTLINE (cppcoreguidelines-pro-bounds-pointer-arithmetic)
    createInterpreter(Args(argv ? argv + 1 : argv, argv + argc));
    m_version = get_stdopt();
    redirect_output();
    init_preamble();
    init_magic();
}

But, what if the argv is nullptr??. It can happen if the user manually removes all the args from kernel.json(including the program name).
It can be show that if we are not passing any argument is same as we are passing only one argument(i.e. the pgroam name).
createInterpreter(Args(nullptr, nullptr)); is same as createInterpreter(Args(argv + 1, argv + 1));.

Proof

Screenshot 2025-04-27 at 12 42 53 PM

@codecov-commenter
Copy link

codecov-commenter commented Apr 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.00%. Comparing base (1158ecd) to head (bd365e7).
Report is 3 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            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     
Files with missing lines Coverage Δ
src/main.cpp 40.67% <100.00%> (ø)
src/xutils.cpp 66.66% <ø> (-8.34%) ⬇️

... and 1 file with indirect coverage changes

Files with missing lines Coverage Δ
src/main.cpp 40.67% <100.00%> (ø)
src/xutils.cpp 66.66% <ø> (-8.34%) ⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@github-actions github-actions bot left a 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

@anutosh491 anutosh491 force-pushed the remove-build-interpreter branch from e71a7eb to cce9ca4 Compare April 27, 2025 07:20
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Collaborator

@anutosh491 anutosh491 left a 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

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 !

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@anutosh491 anutosh491 merged commit 66510a4 into compiler-research:main May 1, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants