Skip to content

[wip] [feature] timeit xmagic enabled for native and lite case#289

Draft
kr-2003 wants to merge 10 commits intocompiler-research:mainfrom
kr-2003:timeit-xmagic
Draft

[wip] [feature] timeit xmagic enabled for native and lite case#289
kr-2003 wants to merge 10 commits intocompiler-research:mainfrom
kr-2003:timeit-xmagic

Conversation

@kr-2003
Copy link
Copy Markdown
Contributor

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

Native

timeit-native.mp4

Lite

timeit-lite.mp4

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 16, 2025

Codecov Report

Attention: Patch coverage is 90.73171% with 19 lines in your changes missing coverage. Please review.

Project coverage is 83.18%. Comparing base (9458ebe) to head (989a66e).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/xmagics/execution.cpp 88.55% 19 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #289      +/-   ##
==========================================
+ Coverage   80.87%   83.18%   +2.30%     
==========================================
  Files          20       22       +2     
  Lines         957     1130     +173     
  Branches       88       98      +10     
==========================================
+ Hits          774      940     +166     
- Misses        183      190       +7     
Files with missing lines Coverage Δ
src/xinterpreter.cpp 91.39% <100.00%> (+0.18%) ⬆️
src/xmagics/execution.hpp 100.00% <100.00%> (ø)
src/xmagics/execution.cpp 88.55% <88.55%> (ø)

... and 2 files with indirect coverage changes

Files with missing lines Coverage Δ
src/xinterpreter.cpp 91.39% <100.00%> (+0.18%) ⬆️
src/xmagics/execution.hpp 100.00% <100.00%> (ø)
src/xmagics/execution.cpp 88.55% <88.55%> (ø)

... and 2 files 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.

@kr-2003 kr-2003 marked this pull request as draft April 16, 2025 18:42
@kr-2003 kr-2003 changed the title [feature] timeit xmagic enabled for native and lite case [wip] [feature] timeit xmagic enabled for native and lite case Apr 16, 2025
@anutosh491
Copy link
Copy Markdown
Collaborator

Thanks a lot for this @kr-2003

Hopefully you were able to get past the output re-direction thingy we were discussing.

Copy link
Copy Markdown
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 68. Check the log or trigger a new build to see more.

Comment thread src/xinterpreter.cpp

void* createInterpreter(const Args &ExtraArgs = {}) {
Args ClangArgs = {/*"-xc++"*/"-v"}; // ? {"-Xclang", "-emit-llvm-only", "-Xclang", "-diagnostic-log-file", "-Xclang", "-", "-xc++"};
void* createInterpreter(const Args& ExtraArgs = {})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: function 'createInterpreter' can be made static or moved into an anonymous namespace to enforce internal linkage [misc-use-internal-linkage]

Suggested change
void* createInterpreter(const Args& ExtraArgs = {})
static void* createInterpreter(const Args& ExtraArgs = {})

Comment thread src/xinterpreter.cpp
ClangArgs.push_back("-isystem");
ClangArgs.push_back(CxxInclude.c_str());
}
if (std::find_if(
Copy link
Copy Markdown
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::find_if" is directly included [misc-include-cleaner]

src/xinterpreter.cpp:21:

- #ifndef EMSCRIPTEN
+ #include <algorithm>
+ #ifndef EMSCRIPTEN

Comment thread src/xinterpreter.cpp
if (std::find_if(
ExtraArgs.begin(),
ExtraArgs.end(),
[](const std::string& s)
Copy link
Copy Markdown
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" is directly included [misc-include-cleaner]

src/xinterpreter.cpp:21:

- #ifndef EMSCRIPTEN
+ #include <string>
+ #ifndef EMSCRIPTEN

Comment thread src/xinterpreter.cpp
)
== ExtraArgs.end())
{
std::string resource_dir = Cpp::DetectResourceDir();
Copy link
Copy Markdown
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 "Cpp::DetectResourceDir" is directly included [misc-include-cleaner]

src/xinterpreter.cpp:21:

- #ifndef EMSCRIPTEN
+ #include <clang/Interpreter/CppInterOp.h>
+ #ifndef EMSCRIPTEN

Comment thread src/xinterpreter.cpp
std::string resource_dir = Cpp::DetectResourceDir();
if (resource_dir.empty())
{
std::cerr << "Failed to detect the resource-dir\n";
Copy link
Copy Markdown
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::cerr" is directly included [misc-include-cleaner]

src/xinterpreter.cpp:21:

- #ifndef EMSCRIPTEN
+ #include <iostream>
+ #ifndef EMSCRIPTEN

Comment thread src/xinterpreter.cpp
ClangArgs.push_back(resource_dir.c_str());
}
std::vector<std::string> CxxSystemIncludes;
Cpp::DetectSystemCompilerIncludePaths(CxxSystemIncludes);
Copy link
Copy Markdown
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 "Cpp::DetectSystemCompilerIncludePaths" is directly included [misc-include-cleaner]

    Cpp::DetectSystemCompilerIncludePaths(CxxSystemIncludes);
         ^

Comment thread src/xinterpreter.cpp
ClangArgs.insert(ClangArgs.end(), ExtraArgs.begin(), ExtraArgs.end());
// FIXME: We should process the kernel input options and conditionally pass
// the gpu args here.
return Cpp::CreateInterpreter(ClangArgs /*, {"-cuda"}*/);
Copy link
Copy Markdown
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 "Cpp::CreateInterpreter" is directly included [misc-include-cleaner]

    return Cpp::CreateInterpreter(ClangArgs /*, {"-cuda"}*/);
                ^

Comment thread src/xinterpreter.cpp
err = Cpp::EndStdStreamCapture();
std::cout << out;
}
struct StreamRedirectRAII
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: class 'StreamRedirectRAII' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

    struct StreamRedirectRAII
           ^

Comment thread src/xinterpreter.cpp
}
struct StreamRedirectRAII
{
std::string& err;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: member 'err' of type 'std::string &' (aka 'basic_string &') is a reference [cppcoreguidelines-avoid-const-or-ref-data-members]

        std::string& err;
                     ^

Comment thread src/xinterpreter.cpp
StreamRedirectRAII(std::string& e)
: err(e)
{
Cpp::BeginStdStreamCapture(Cpp::kStdErr);
Copy link
Copy Markdown
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 "Cpp::BeginStdStreamCapture" is directly included [misc-include-cleaner]

            Cpp::BeginStdStreamCapture(Cpp::kStdErr);
                 ^

@kr-2003
Copy link
Copy Markdown
Contributor Author

kr-2003 commented Apr 16, 2025

Thanks a lot for this @kr-2003

Hopefully you were able to get past the output re-direction thingy we were discussing.

Yes, there are some things left to do here. Although the feature is working fine for now.
I'll see the following remaining tasks tomorrow:

  1. CI Tests failing
  2. Test coverage for timeit
  3. Other code suggestions by github-actions bot

Copy link
Copy Markdown
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 60. Check the log or trigger a new build to see more.

Comment thread src/xinterpreter.cpp
StreamRedirectRAII(std::string& e)
: err(e)
{
Cpp::BeginStdStreamCapture(Cpp::kStdErr);
Copy link
Copy Markdown
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 "Cpp::kStdErr" is directly included [misc-include-cleaner]

            Cpp::BeginStdStreamCapture(Cpp::kStdErr);
                                            ^

Comment thread src/xinterpreter.cpp
: err(e)
{
Cpp::BeginStdStreamCapture(Cpp::kStdErr);
Cpp::BeginStdStreamCapture(Cpp::kStdOut);
Copy link
Copy Markdown
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 "Cpp::kStdOut" is directly included [misc-include-cleaner]

            Cpp::BeginStdStreamCapture(Cpp::kStdOut);
                                            ^

Comment thread src/xinterpreter.cpp

~StreamRedirectRAII()
{
std::string out = Cpp::EndStdStreamCapture();
Copy link
Copy Markdown
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 "Cpp::EndStdStreamCapture" is directly included [misc-include-cleaner]

            std::string out = Cpp::EndStdStreamCapture();
                                   ^

Comment thread src/xinterpreter.cpp
{
std::string out = Cpp::EndStdStreamCapture();
err = Cpp::EndStdStreamCapture();
std::cout << out;
Copy link
Copy Markdown
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::cout" is directly included [misc-include-cleaner]

            std::cout << out;
                 ^

Comment thread src/xinterpreter.cpp
interpreter::interpreter(int argc, const char* const* argv) :
xmagics()
interpreter::interpreter(int argc, const char* const* argv)
: xmagics()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: initializer for member 'xmagics' is redundant [readability-redundant-member-init]

Suggested change
: xmagics()
:

Comment thread src/xinterpreter.cpp
auto found1 = found++;
while (isspace(code[++found1])) ;
return xeus::create_is_complete_reply("incomplete", code.substr(found, found1-found));
while (isspace(code[++found1]))
Copy link
Copy Markdown
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 "isspace" is directly included [misc-include-cleaner]

src/xinterpreter.cpp:21:

- #ifndef EMSCRIPTEN
+ #include <cctype>
+ #ifndef EMSCRIPTEN

Comment thread src/xmagics/execution.cpp

#include "execution.hpp"
#include "xeus-cpp/xinterpreter.hpp"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Suggested change

Comment thread src/xmagics/execution.cpp

namespace xcpp
{
struct StreamRedirectRAII
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: class 'StreamRedirectRAII' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]

    struct StreamRedirectRAII
           ^

Comment thread src/xmagics/execution.cpp
{
struct StreamRedirectRAII
{
std::string& err;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: member 'err' of type 'std::string &' (aka 'basic_string &') is a reference [cppcoreguidelines-avoid-const-or-ref-data-members]

        std::string& err;
                     ^

Comment thread src/xmagics/execution.cpp
{
struct StreamRedirectRAII
{
std::string& err;
Copy link
Copy Markdown
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" is directly included [misc-include-cleaner]

src/xmagics/execution.cpp:13:

+ #include <string>

Copy link
Copy Markdown
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 48. Check the log or trigger a new build to see more.

Comment thread src/xmagics/execution.cpp
{
std::string out = Cpp::EndStdStreamCapture();
err = Cpp::EndStdStreamCapture();
std::cout << out;
Copy link
Copy Markdown
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::cout" is directly included [misc-include-cleaner]

src/xmagics/execution.cpp:13:

+ #include <iostream>

Comment thread src/xmagics/execution.cpp Outdated
Comment thread src/xmagics/execution.cpp
{
}

void timeit::get_options(argparser& argpars)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: method 'get_options' can be made static [readability-convert-member-functions-to-static]

src/xmagics/execution.hpp:47:

-         void get_options(argparser& argpars);
+         static void get_options(argparser& argpars);

Comment thread src/xmagics/execution.cpp
{
}

void timeit::get_options(argparser& argpars)
Copy link
Copy Markdown
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 "xcpp::argparser" is directly included [misc-include-cleaner]

src/xmagics/execution.cpp:12:

- #include "clang/Interpreter/CppInterOp.h"
+ #include "xeus-cpp/xoptions.hpp"
+ #include "clang/Interpreter/CppInterOp.h"

Comment thread src/xmagics/execution.cpp
.nargs(0);
}

std::string timeit::inner(std::size_t number, const std::string& code, int exec_counter) const
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: method 'inner' can be made static [readability-convert-member-functions-to-static]

src/xmagics/execution.hpp:48:

-         std::string inner(std::size_t number, const std::string& code, int exec_counter) const;
+         static std::string inner(std::size_t number, const std::string& code, int exec_counter) ;
Suggested change
std::string timeit::inner(std::size_t number, const std::string& code, int exec_counter) const
std::string timeit::inner(std::size_t number, const std::string& code, int exec_counter)

Comment thread src/xmagics/execution.cpp
.nargs(0);
}

std::string timeit::inner(std::size_t number, const std::string& code, int exec_counter) const
Copy link
Copy Markdown
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::size_t" is directly included [misc-include-cleaner]

src/xmagics/execution.cpp:13:

+ #include <cstddef>

Comment thread src/xmagics/execution.cpp
std::string timeit::inner(std::size_t number, const std::string& code, int exec_counter) const
{
static std::size_t counter = 0; // Ensure unique lambda names
std::string unique_id = std::to_string(counter++);
Copy link
Copy Markdown
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::to_string" is directly included [misc-include-cleaner]

        std::string unique_id = std::to_string(counter++);
                                     ^

Comment thread src/xmagics/execution.cpp
{
static std::size_t counter = 0; // Ensure unique lambda names
std::string unique_id = std::to_string(counter++);
std::string timeit_code = "";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: redundant string initialization [readability-redundant-string-init]

Suggested change
std::string timeit_code = "";
std::string timeit_code;

Comment thread src/xmagics/execution.cpp
timeit_code += "auto user_code_" + unique_id + " = []() {\n";
timeit_code += " " + code + "\n";
timeit_code += "};\n";
timeit_code += "get_elapsed_time_" + std::to_string(exec_counter) + "(" + std::to_string(number)
Copy link
Copy Markdown
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::to_string" is directly included [misc-include-cleaner]

        timeit_code += "get_elapsed_time_" + std::to_string(exec_counter) + "(" + std::to_string(number)
                                                  ^

Comment thread src/xmagics/execution.cpp
return timeit_code;
}

std::string timeit::_format_time(double timespan, std::size_t precision) const
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: method '_format_time' can be made static [readability-convert-member-functions-to-static]

src/xmagics/execution.hpp:49:

-         std::string _format_time(double timespan, std::size_t precision) const;
+         static std::string _format_time(double timespan, std::size_t precision) ;
Suggested change
std::string timeit::_format_time(double timespan, std::size_t precision) const
std::string timeit::_format_time(double timespan, std::size_t precision)

Copy link
Copy Markdown
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 39. Check the log or trigger a new build to see more.

Comment thread src/xmagics/execution.cpp

std::string timeit::_format_time(double timespan, std::size_t precision) const
{
std::vector<std::string> units{"s", "ms", "us", "ns"};
Copy link
Copy Markdown
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::vector" is directly included [misc-include-cleaner]

src/xmagics/execution.cpp:13:

+ #include <vector>

Comment thread src/xmagics/execution.cpp
{
std::vector<std::string> units{"s", "ms", "us", "ns"};
std::vector<double> scaling{1, 1e3, 1e6, 1e9};
std::ostringstream output;
Copy link
Copy Markdown
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::ostringstream" is directly included [misc-include-cleaner]

src/xmagics/execution.cpp:13:

+ #include <sstream>

Comment thread src/xmagics/execution.cpp
std::vector<std::string> units{"s", "ms", "us", "ns"};
std::vector<double> scaling{1, 1e3, 1e6, 1e9};
std::ostringstream output;
int order;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: variable 'order' is not initialized [cppcoreguidelines-init-variables]

Suggested change
int order;
int order = 0;

Comment thread src/xmagics/execution.cpp

if (timespan > 0.0)
{
order = std::min(-static_cast<int>(std::floor(std::floor(std::log10(timespan)) / 3)), 3);
Copy link
Copy Markdown
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::floor" is directly included [misc-include-cleaner]

src/xmagics/execution.cpp:13:

+ #include <cmath>

Comment thread src/xmagics/execution.cpp

if (timespan > 0.0)
{
order = std::min(-static_cast<int>(std::floor(std::floor(std::log10(timespan)) / 3)), 3);
Copy link
Copy Markdown
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::log10" is directly included [misc-include-cleaner]

            order = std::min(-static_cast<int>(std::floor(std::floor(std::log10(timespan)) / 3)), 3);
                                                                          ^

Comment thread src/xmagics/execution.cpp

if (timespan > 0.0)
{
order = std::min(-static_cast<int>(std::floor(std::floor(std::log10(timespan)) / 3)), 3);
Copy link
Copy Markdown
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::min" is directly included [misc-include-cleaner]

src/xmagics/execution.cpp:13:

+ #include <algorithm>

Comment thread src/xmagics/execution.cpp
{
order = 3;
}
output.precision(precision);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: narrowing conversion from 'std::size_t' (aka 'unsigned long') to signed type 'streamsize' (aka 'long') is implementation-defined [cppcoreguidelines-narrowing-conversions]

        output.precision(precision);
                         ^

Comment thread src/xmagics/execution.cpp
void timeit::execute(std::string& line, std::string& cell)
{
exec_counter++;
argparser argpars("timeit", XEUS_CPP_VERSION, argparse::default_arguments::none);
Copy link
Copy Markdown
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_VERSION" is directly included [misc-include-cleaner]

src/xmagics/execution.cpp:9:

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

Comment thread src/xmagics/execution.cpp
void timeit::execute(std::string& line, std::string& cell)
{
exec_counter++;
argparser argpars("timeit", XEUS_CPP_VERSION, argparse::default_arguments::none);
Copy link
Copy Markdown
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 "argparse::default_arguments" is directly included [misc-include-cleaner]

src/xmagics/execution.cpp:13:

+ #include <argparse/argparse.hpp>

Comment thread src/xmagics/execution.cpp
code += " " + s;
}
}
catch (std::logic_error& e)
Copy link
Copy Markdown
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::logic_error" is directly included [misc-include-cleaner]

src/xmagics/execution.cpp:13:

+ #include <stdexcept>

Copy link
Copy Markdown
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 30. Check the log or trigger a new build to see more.

Comment thread src/xmagics/execution.cpp
void timeit::operator()(const std::string& line)
{
std::string cline = line;
std::string ccell = "";
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: redundant string initialization [readability-redundant-string-init]

Suggested change
std::string ccell = "";
std::string ccell;

Comment thread src/xmagics/execution.cpp
execute(cline, ccell);
}

void timeit::get_options(argparser& argpars)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: method 'get_options' can be made static [readability-convert-member-functions-to-static]

src/xmagics/execution.hpp:35:

-         void get_options(argparser& argpars);
+         static void get_options(argparser& argpars);

Comment thread src/xmagics/execution.cpp
.nargs(0);
}

std::string timeit::inner(std::size_t number, const std::string& code, int exec_counter) const
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: method 'inner' can be made static [readability-convert-member-functions-to-static]

src/xmagics/execution.hpp:36:

-         std::string inner(std::size_t number, const std::string& code, int exec_counter) const;
+         static std::string inner(std::size_t number, const std::string& code, int exec_counter) ;
Suggested change
std::string timeit::inner(std::size_t number, const std::string& code, int exec_counter) const
std::string timeit::inner(std::size_t number, const std::string& code, int exec_counter)

Comment thread src/xmagics/execution.cpp
return timeit_code;
}

std::string timeit::_format_time(double timespan, std::size_t precision) const
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: method '_format_time' can be made static [readability-convert-member-functions-to-static]

src/xmagics/execution.hpp:37:

-         std::string _format_time(double timespan, std::size_t precision) const;
+         static std::string _format_time(double timespan, std::size_t precision) ;
Suggested change
std::string timeit::_format_time(double timespan, std::size_t precision) const
std::string timeit::_format_time(double timespan, std::size_t precision)

Comment thread src/xmagics/execution.cpp
{
if (trim(cell).empty() && (argpars["-h"] == false))
{
std::cerr << "No expression given to evaluate" << std::endl;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: do not use 'std::endl' with streams; use '\n' instead [performance-avoid-endl]

Suggested change
std::cerr << "No expression given to evaluate" << std::endl;
std::cerr << "No expression given to evaluate" << '\n';

Comment thread src/xmagics/execution.cpp
{
if (trim(cell).empty() && (argpars["-h"] == false))
{
std::cerr << "No expression given to evaluate" << std::endl;
Copy link
Copy Markdown
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::cerr" is directly included [misc-include-cleaner]

                std::cerr << "No expression given to evaluate" << std::endl;
                     ^

Comment thread src/xmagics/execution.cpp
{
if (trim(cell).empty() && (argpars["-h"] == false))
{
std::cerr << "No expression given to evaluate" << std::endl;
Copy link
Copy Markdown
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::endl" is directly included [misc-include-cleaner]

src/xmagics/execution.cpp:13:

+ #include <ostream>

Comment thread src/xmagics/execution.cpp Outdated
bool hadError = false;

bool compilation_result = true;
compilation_result = Cpp::Process("#include <chrono>\n#include <iostream>\n");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: Value stored to 'compilation_result' is never read [clang-analyzer-deadcode.DeadStores]

        compilation_result = Cpp::Process("#include <chrono>\n#include <iostream>\n");
        ^
Additional context

src/xmagics/execution.cpp:156: Value stored to 'compilation_result' is never read

        compilation_result = Cpp::Process("#include <chrono>\n#include <iostream>\n");
        ^

Comment thread src/xmagics/execution.cpp
}
)";

compilation_result = Cpp::Process(timing_function.c_str());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: Value stored to 'compilation_result' is never read [clang-analyzer-deadcode.DeadStores]

        compilation_result = Cpp::Process(timing_function.c_str());
        ^
Additional context

src/xmagics/execution.cpp:172: Value stored to 'compilation_result' is never read

        compilation_result = Cpp::Process(timing_function.c_str());
        ^

Comment thread src/xmagics/execution.cpp
try
{
StreamRedirectRAII R(err);
std::ostringstream buffer_out, buffer_err;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]

Suggested change
std::ostringstream buffer_out, buffer_err;
std::ostringstream buffer_out;
std::ostringstream buffer_err;

Copy link
Copy Markdown
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 21. Check the log or trigger a new build to see more.

Comment thread src/xmagics/execution.cpp
bool hadError = false;

bool compilation_result = true;
compilation_result = Cpp::Process("#include <chrono>\n");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: Value stored to 'compilation_result' is never read [clang-analyzer-deadcode.DeadStores]

        compilation_result = Cpp::Process("#include <chrono>\n");
        ^
Additional context

src/xmagics/execution.cpp:156: Value stored to 'compilation_result' is never read

        compilation_result = Cpp::Process("#include <chrono>\n");
        ^

Comment thread src/xmagics/execution.cpp
{
StreamRedirectRAII R(err);
std::ostringstream buffer_out, buffer_err;
std::streambuf* old_cout = std::cout.rdbuf(buffer_out.rdbuf());
Copy link
Copy Markdown
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::streambuf" is directly included [misc-include-cleaner]

src/xmagics/execution.cpp:13:

+ #include <streambuf>

Comment thread src/xmagics/execution.cpp
std::cout.rdbuf(old_cout);
std::cerr.rdbuf(old_cerr);
}
catch (std::exception& e)
Copy link
Copy Markdown
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::exception" is directly included [misc-include-cleaner]

src/xmagics/execution.cpp:13:

+ #include <exception>

Comment thread src/xmagics/execution.cpp
{
for (std::size_t n = 0; n < 10; ++n)
{
number = std::pow(10, n);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: narrowing conversion from 'typename __gnu_cxx::__promote_2<int, unsigned long>::__type' (aka 'double') to 'int' [cppcoreguidelines-narrowing-conversions]

                number = std::pow(10, n);
                         ^

Comment thread src/xmagics/execution.cpp
{
for (std::size_t n = 0; n < 10; ++n)
{
number = std::pow(10, n);
Copy link
Copy Markdown
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::pow" is directly included [misc-include-cleaner]

                number = std::pow(10, n);
                              ^

Comment thread src/xmagics/execution.cpp
{
number = std::pow(10, n);
std::string timeit_code = inner(number, code, exec_counter);
std::ostringstream buffer_out, buffer_err;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]

Suggested change
std::ostringstream buffer_out, buffer_err;
std::ostringstream buffer_out;
std::ostringstream buffer_err;

Comment thread src/xmagics/execution.cpp
auto res_ptr = Cpp::Evaluate(timeit_code.c_str(), &hadError);
std::cout.rdbuf(old_cout);
std::cerr.rdbuf(old_cerr);
output = std::to_string(res_ptr);
Copy link
Copy Markdown
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::to_string" is directly included [misc-include-cleaner]

                output = std::to_string(res_ptr);
                              ^

Comment thread src/xmagics/execution.cpp
std::cerr.rdbuf(old_cerr);
output = std::to_string(res_ptr);
err += buffer_err.str();
double elapsed_time = std::stod(output) * 1e-6;
Copy link
Copy Markdown
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::stod" is directly included [misc-include-cleaner]

                double elapsed_time = std::stod(output) * 1e-6;
                                           ^

Comment thread src/xmagics/execution.cpp
for (std::size_t r = 0; r < static_cast<std::size_t>(repeat); ++r)
{
std::string timeit_code = inner(number, code, exec_counter);
std::ostringstream buffer_out, buffer_err;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]

Suggested change
std::ostringstream buffer_out, buffer_err;
std::ostringstream buffer_out;
std::ostringstream buffer_err;

Comment thread src/xmagics/execution.cpp
{
stdev += (all_runs[r] - mean) * (all_runs[r] - mean);
}
stdev = std::sqrt(stdev / repeat);
Copy link
Copy Markdown
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::sqrt" is directly included [misc-include-cleaner]

        stdev = std::sqrt(stdev / repeat);
                     ^

Copy link
Copy Markdown
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 11. Check the log or trigger a new build to see more.

Comment thread src/xmagics/execution.cpp

std::cout << _format_time(mean, precision) << " +- " << _format_time(stdev, precision);
std::cout << " per loop (mean +- std. dev. of " << repeat << " run" << ((repeat == 1) ? ", " : "s ");
std::cout << number << " loop" << ((number == 1) ? "" : "s") << " each)" << std::endl;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: do not use 'std::endl' with streams; use '\n' instead [performance-avoid-endl]

Suggested change
std::cout << number << " loop" << ((number == 1) ? "" : "s") << " each)" << std::endl;
std::cout << number << " loop" << ((number == 1) ? "" : "s") << " each)" << '\n';

Comment thread src/xmagics/execution.cpp
std::cout << " per loop (mean +- std. dev. of " << repeat << " run" << ((repeat == 1) ? ", " : "s ");
std::cout << number << " loop" << ((number == 1) ? "" : "s") << " each)" << std::endl;
}
} No newline at end of file
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: namespace 'xcpp' not terminated with a closing comment [llvm-namespace-comment]

src/xmagics/execution.cpp:-1:

+  // namespace xcpp
Additional context

src/xmagics/execution.cpp:14: namespace 'xcpp' starts here

namespace xcpp
          ^

Comment thread src/xmagics/execution.hpp
************************************************************************************/

#ifndef XMAGICS_EXECUTION_HPP
#define XMAGICS_EXECUTION_HPP
Copy link
Copy Markdown
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 XMAGICS_EXECUTION_HPP
#ifndef GITHUB_WORKSPACE_SRC_XMAGICS_EXECUTION_HPP
#define GITHUB_WORKSPACE_SRC_XMAGICS_EXECUTION_HPP

Comment thread src/xmagics/execution.hpp
{
public:

XEUS_CPP_API
Copy link
Copy Markdown
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/execution.hpp:14:

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

Comment thread src/xmagics/execution.hpp
public:

XEUS_CPP_API
virtual void operator()(const std::string& line) override;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: 'virtual' is redundant since the function is already declared 'override' [cppcoreguidelines-explicit-virtual-functions]

Suggested change
virtual void operator()(const std::string& line) override;
void operator()(const std::string& line) override;

Comment thread src/xmagics/execution.hpp
virtual void operator()(const std::string& line) override;

XEUS_CPP_API
virtual void operator()(const std::string& line, const std::string& cell) override;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: 'virtual' is redundant since the function is already declared 'override' [cppcoreguidelines-explicit-virtual-functions]

Suggested change
virtual void operator()(const std::string& line, const std::string& cell) override;
void operator()(const std::string& line, const std::string& cell) override;

Comment thread src/xmagics/execution.hpp
XEUS_CPP_API
virtual void operator()(const std::string& line, const std::string& cell) override;

public:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: redundant access specifier has the same accessibility as the previous access specifier [readability-redundant-access-specifiers]

Suggested change
public:
Additional context

src/xmagics/execution.hpp:21: previously declared here

    public:
    ^

Comment thread src/xmagics/execution.hpp
private:

void get_options(argparser& argpars);
std::string inner(std::size_t number, const std::string& code, int exec_counter) const;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: function 'inner' should be marked [[nodiscard]] [modernize-use-nodiscard]

Suggested change
std::string inner(std::size_t number, const std::string& code, int exec_counter) const;
[[nodiscard]] std::string inner(std::size_t number, const std::string& code, int exec_counter) const;

Comment thread src/xmagics/execution.hpp

void get_options(argparser& argpars);
std::string inner(std::size_t number, const std::string& code, int exec_counter) const;
std::string _format_time(double timespan, std::size_t precision) const;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: function '_format_time' should be marked [[nodiscard]] [modernize-use-nodiscard]

Suggested change
std::string _format_time(double timespan, std::size_t precision) const;
[[nodiscard]] std::string _format_time(double timespan, std::size_t precision) const;

Comment thread src/xmagics/execution.hpp

void get_options(argparser& argpars);
std::string inner(std::size_t number, const std::string& code, int exec_counter) const;
std::string _format_time(double timespan, std::size_t precision) const;
Copy link
Copy Markdown
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 function '_format_time' [readability-identifier-naming]

Suggested change
std::string _format_time(double timespan, std::size_t precision) const;
std::string format_time(double timespan, std::size_t precision) const;

@anutosh491
Copy link
Copy Markdown
Collaborator

The effort here is good but the it won't be too ideal to keep the workaround introduced here in the source code for long.

I think this llvm/llvm-project#84769 would go in sometime soon (we have llvm 20.1.5 release planned on May 13th and llvm 20.1.6 planned on May 27th . Having it through one of these would be great)

In the meanwhile would you like to have a look at these (and try having draft PRs for these ?)

  1. [Feature Request]: Support cling/clang-repl Value equivalent through CppInterop CppInterOp#418
    followed by
  2. Implement last value printing #2

Copy link
Copy Markdown
Contributor

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

This PR seem to have (almost) no tests...

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.

4 participants