Skip to content

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 !

@github-actions
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