Skip to content

Conversation

Vipul-Cariappa
Copy link
Collaborator

Description

Implements a way to create sub-interpreter and use them with in a notebook session.

Fixes # (issue)

Type of change

Please tick all options which are relevant.

  • Bug fix
  • New feature
  • Added/removed dependencies
  • Required documentation updates

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

There were too many comments to post at once. Showing the first 10 out of 12. Check the log or trigger a new build to see more.

#include "xsystem.hpp"

#include "xmagics/multi_interpreter.hpp"

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: included header multi_interpreter.hpp is not used directly [misc-include-cleaner]

Suggested change

#include "multi_interpreter.hpp"

#include <iostream>
#include <iterator>
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: included header iostream is not used directly [misc-include-cleaner]

Suggested change
#include <iterator>
#include <iterator>


#include <iostream>
#include <iterator>
#include <sstream>
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: included header iterator is not used directly [misc-include-cleaner]

Suggested change
#include <sstream>
#include <sstream>

#include <sstream>
#include <string>
#include <vector>

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: included header vector is not used directly [misc-include-cleaner]

Suggested change


static std::vector<std::string> split(const std::string& input)
{
std::istringstream buffer(input);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: unused local variable 'buffer' of type 'std::istringstream' (aka 'basic_istringstream') [bugprone-unused-local-non-trivial-variable]

    std::istringstream buffer(input);
                       ^

return ret;
}

static constexpr size_t len(std::string_view n)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "size_t" is directly included [misc-include-cleaner]

src/xmagics/multi_interpreter.cpp:10:

- #include <iostream>
+ #include <cstddef>
+ #include <iostream>

return ret;
}

static constexpr size_t len(std::string_view n)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "std::string_view" is directly included [misc-include-cleaner]

src/xmagics/multi_interpreter.cpp:14:

- #include <vector>
+ #include <string_view>
+ #include <vector>

************************************************************************************/

#ifndef XEUS_CPP_MULTI_INTERPRETER_MAGIC_HPP
#define XEUS_CPP_MULTI_INTERPRETER_MAGIC_HPP
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: header guard does not follow preferred style [llvm-header-guard]

Suggested change
#define XEUS_CPP_MULTI_INTERPRETER_MAGIC_HPP
#ifndef GITHUB_WORKSPACE_SRC_XMAGICS_MULTI_INTERPRETER_HPP
#define GITHUB_WORKSPACE_SRC_XMAGICS_MULTI_INTERPRETER_HPP

src/xmagics/multi_interpreter.hpp:32:

- #endif  // XEUS_CPP_MULTI_INTERPRETER_MAGIC_HPP
+ #endif // GITHUB_WORKSPACE_SRC_XMAGICS_MULTI_INTERPRETER_HPP


#include <string>
#include <unordered_map>

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: included header unordered_map is not used directly [misc-include-cleaner]

Suggested change

#include <string>
#include <unordered_map>

#include "CppInterOp/CppInterOp.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'CppInterOp/CppInterOp.h' file not found [clang-diagnostic-error]

#include "CppInterOp/CppInterOp.h"
         ^

@mcbarton mcbarton mentioned this pull request Sep 26, 2025
4 tasks
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

{
public:

XEUS_CPP_API
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: no header providing "XEUS_CPP_API" is directly included [misc-include-cleaner]

src/xmagics/multi_interpreter.hpp:15:

- #include "xeus-cpp/xmagics.hpp"
+ #include "xeus-cpp/xeus_cpp_config.hpp"
+ #include "xeus-cpp/xmagics.hpp"


private:

std::unordered_map<std::string, Cpp::TInterp_t> interpreters;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: invalid case style for private member 'interpreters' [readability-identifier-naming]

Suggested change
std::unordered_map<std::string, Cpp::TInterp_t> interpreters;
std::unordered_map<std::string, Cpp::TInterp_t> m_interpreters;

@mcbarton
Copy link
Collaborator

Please add some tests to this PR

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.

2 participants