Formatting/additions/etc #31
Conversation
PhilipDeegan
force-pushed
the
pee_arr
branch
3 times, most recently
from
d2cf18c to
a45591f
Compare
|
Warning Rate Limit Exceeded@PhilipDeegan has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 3 minutes and 45 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR. We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 18
Files ignored due to filter (1)
- mkn.yaml
Files selected for processing (39)
- .github/workflows/build_win.yml (1 hunks)
- .sublime-project (1 hunks)
- .sublime-project.sublime-workspace (1 hunks)
- inc/mkn/kul/all.hpp (9 hunks)
- inc/mkn/kul/asio/log.hpp (2 hunks)
- inc/mkn/kul/assert.hpp (2 hunks)
- inc/mkn/kul/cli.hpp (1 hunks)
- inc/mkn/kul/dbg.hpp (1 hunks)
- inc/mkn/kul/except.hpp (1 hunks)
- inc/mkn/kul/for.hpp (2 hunks)
- inc/mkn/kul/log.hpp (3 hunks)
- inc/mkn/kul/math.hpp (3 hunks)
- inc/mkn/kul/os.hpp (1 hunks)
- inc/mkn/kul/os/nixish/ipc.hpp (1 hunks)
- inc/mkn/kul/os/nixish/os.bot.hpp (1 hunks)
- inc/mkn/kul/os/nixish/src/os/dir/files.ipp (1 hunks)
- inc/mkn/kul/os/nixish/src/proc/run.ipp (1 hunks)
- inc/mkn/kul/os/nixish/src/signal/handler.ipp (1 hunks)
- inc/mkn/kul/os/nixish/src/signal/stacktrace.ipp (1 hunks)
- inc/mkn/kul/os/nixish/sys.hpp (2 hunks)
- inc/mkn/kul/os/threads.hpp (3 hunks)
- inc/mkn/kul/os/win/ipc.hpp (5 hunks)
- inc/mkn/kul/os/win/os.bot.hpp (1 hunks)
- inc/mkn/kul/os/win/src/proc/run.cpp (2 hunks)
- inc/mkn/kul/os/win/src/signal/se_handler.cpp (1 hunks)
- inc/mkn/kul/proc.hpp (2 hunks)
- inc/mkn/kul/scm.hpp (10 hunks)
- inc/mkn/kul/serial/cli.arg.end.hpp (2 hunks)
- inc/mkn/kul/sort.hpp (1 hunks)
- inc/mkn/kul/span.hpp (2 hunks)
- inc/mkn/kul/sys.hpp (1 hunks)
- inc/mkn/kul/threads.hpp (2 hunks)
- inc/mkn/kul/tuple.hpp (1 hunks)
- inc/mkn/kul/yaml.hpp (1 hunks)
- res/bench/sort.cpp (1 hunks)
- tst/test.cpp (2 hunks)
- tst/test/sort.ipp (1 hunks)
- tst/test/string.ipp (2 hunks)
- tst/usage.cpp (2 hunks)
Files skipped from review due to trivial changes (21)
- .sublime-project
- .sublime-project.sublime-workspace
- inc/mkn/kul/cli.hpp
- inc/mkn/kul/dbg.hpp
- inc/mkn/kul/except.hpp
- inc/mkn/kul/os.hpp
- inc/mkn/kul/os/nixish/ipc.hpp
- inc/mkn/kul/os/nixish/src/os/dir/files.ipp
- inc/mkn/kul/os/nixish/src/proc/run.ipp
- inc/mkn/kul/os/nixish/src/signal/handler.ipp
- inc/mkn/kul/os/nixish/src/signal/stacktrace.ipp
- inc/mkn/kul/os/threads.hpp
- inc/mkn/kul/os/win/ipc.hpp
- inc/mkn/kul/os/win/src/proc/run.cpp
- inc/mkn/kul/os/win/src/signal/se_handler.cpp
- inc/mkn/kul/serial/cli.arg.end.hpp
- inc/mkn/kul/span.hpp
- inc/mkn/kul/sys.hpp
- inc/mkn/kul/yaml.hpp
- tst/test.cpp
- tst/test/string.ipp
Additional comments (Suppressed): 40
.github/workflows/build_win.yml (2)
15-18: The addition of
ilammy/msvc-dev-cmd@v1witharch: amd64is a good move as it sets up the MSVC environment for the GitHub Actions job. This replaces the manual setup of the MSVC environment in the old hunk. Ensure that this change doesn't affect the build process negatively.19-26: The build/test step remains largely unchanged, except for the removal of the manual MSVC environment setup. The use of
ilammy/msvc-dev-cmd@v1should handle this setup now. Verify that the build and test process still works as expected after this change.inc/mkn/kul/os/nixish/os.bot.hpp (1)
- 111-116: The
homefunction now accepts astd::stringparameter, which allows for more flexibility in specifying the application directory within the home directory. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, consider the implications of this change on the system or user experience, such as potential changes to the directory structure or the need for additional error handling if the specified application directory does not exist.inc/mkn/kul/assert.hpp (2)
31-38: The inclusion of
<cstdlib>is new in this hunk. Ensure that this inclusion is necessary and does not conflict with other parts of the codebase. Also, the indentation of the#includedirective seems inconsistent with the rest of the code.54-56: The
abort_iffunction is a new addition. It's a simple utility function that callsstd::abort()if the input booleanbistrue. Ensure that this function is used correctly throughout the codebase, as it will terminate the program immediately without executing any cleanup routines or destructors.inc/mkn/kul/asio/log.hpp (2)
41-46: The
Exceptionclass remains unchanged in this hunk. It continues to inherit frommkn::kul::Exceptionand takes the same parameters for its constructor. No issues found.117-130: The logging macros
KASIO_LOG_INF,KASIO_LOG_ERR,KASIO_LOG_DBG,KASIO_LOG_TRChave been updated to use the fully qualified namespacemkn::kul::asio::LogMessage. This change improves the clarity of the code by making it explicit whereLogMessageis defined. However, ensure that this change does not conflict with any otherLogMessagedefinitions in the codebase.inc/mkn/kul/for.hpp (5)
57-68: The
for_Nfunction has been reformatted for better readability. The logic remains the same, it still applies a functionfnto a sequence of integers from 0 toN-1at compile time. The function signature and behavior remain unchanged.80-83: The
generatefunction is overloaded to generate a vector of values by applying a functionfto a sequence of integers from 0 tocount. It simply calls the previousgeneratefunction withfromset to 0. This function is well-written and efficient.85-93: The
generatefunction is overloaded to generate a vector of values by applying a functionfto each value in a given container. It first reserves space for the vector if the container is not empty, then it fills the vector by applying the functionfto each value in the container. This function is well-written and efficient.95-109: The
generatefunction is overloaded to generate an array of values by applying a functionfto each value in a given array. It uses a helper functiongenerate_array_to apply the functionfto each value in the array and return a new array of the results. This function is well-written and efficient.111-114: The
generatefunction is overloaded to generate a vector or array of values by applying a functionfto each value in a given vector or array. It simply calls the previousgeneratefunction with the same parameters. This function is well-written and efficient.inc/mkn/kul/math.hpp (3)
32-39: The inclusion of
<numeric>and<algorithm>headers is new. Ensure that these are necessary for the new functions introduced in this file. If they are not used, consider removing them to reduce unnecessary dependencies.67-83: The
productandsumfunctions are new additions. They usestd::accumulatefrom the<numeric>header to calculate the product and sum of all elements in a container. The code looks correct and efficient. However, consider adding error handling for empty containers to prevent potential issues.template<typename Container, typename Multiplies = typename Container::value_type> Multiplies product(Container const& container, Multiplies mul = 1) { + if (container.empty()) { + throw std::invalid_argument("Container is empty"); + } return std::accumulate(container.begin(), container.end(), mul, std::multiplies<Multiplies>()); } template<typename Container, typename Return = typename Container::value_type> Return sum(Container const& container, Return r = 0) { + if (container.empty()) { + throw std::invalid_argument("Container is empty"); + } return std::accumulate(container.begin(), container.end(), r); }
- 44-48: The
Exceptionclass is unchanged. It inherits frommkn::kul::Exceptionand is used for exception handling in this file.tst/test/sort.ipp (12)
3-12: The test case
QSort, ascendingis well written and checks if the sorting function works correctly for a vector in ascending order. The test case is clear and concise.14-25: The test case
QSort, halfchecks if the sorting function works correctly for a half-sorted vector. The test case is clear and concise.27-37: The test case
QSort, descendingchecks if the sorting function works correctly for a vector in descending order. The test case is clear and concise.39-52: The test case
QSort, parallel_ascending0checks if the parallel sorting function works correctly for a vector in ascending order. The test case is clear and concise.54-67: The test case
QSort, parallel_ascending1checks if the parallel sorting function works correctly for a vector in ascending order. The test case is clear and concise.69-78: The test case
QSort, parallel_ascending2checks if the parallel sorting function works correctly for a vector in ascending order. The test case is clear and concise.80-93: The test case
QSort, parallel_ascending3checks if the parallel sorting function works correctly for a vector in ascending order. The test case is clear and concise.95-108: The test case
QSort, parallel_ascending4checks if the parallel sorting function works correctly for a vector in ascending order. The test case is clear and concise.110-126: The lambda functions
arr_LT_fnandarr_GT_fnare well written and clear. They are used to compare arrays in the following test cases.128-144: The test case
QSort, ascending_arraychecks if the sorting function works correctly for a vector of arrays in ascending order. The test case is clear and concise.146-162: The test case
QSort, ascending_array1checks if the sorting function works correctly for a vector of arrays in ascending order. The test case is clear and concise.164-185: The test case
QSort, para_ascending_array1checks if the parallel sorting function works correctly for a vector of arrays in ascending order. The test case is clear and concise.inc/mkn/kul/proc.hpp (2)
57-64: No changes have been made in this hunk. The
ExceptionandExitExceptionclasses remain the same.80-93: No significant changes have been made in this hunk. The only change is the addition of whitespace for readability, which does not affect the functionality of the code.
inc/mkn/kul/all.hpp (2)
78-80: The
compare_tofunction is recursively calling itself, which will lead to a stack overflow. This seems to be a mistake. Please verify the intended behavior.88-90: The
compare_tofunction is recursively calling itself, which will lead to a stack overflow. This seems to be a mistake. Please verify the intended behavior.inc/mkn/kul/tuple.hpp (1)
- 31-37: The
vectorheader file has been replaced withtuple. Ensure that this change does not affect other parts of the code that might be relying onvectorfrom this header file.tst/usage.cpp (2)
154-160: No changes detected in this hunk. The code remains the same as in the old hunk.
221-228: The changes in this hunk are purely stylistic, involving the reformatting of the initialization of
argV. The logic and functionality remain the same.inc/mkn/kul/os/nixish/sys.hpp (3)
53-59: The constructor of
SharedLibrarynow accepts an additional parameterflagswith a default value of__MKN_KUL_SYS_DLOPEN__. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature or are compatible with the default value.78-82: The constructor of
SharedFunctionnow throws an exception ifdlsymfails. This is a change in behavior from the old code, which would allow the creation of aSharedFunctionwith a null function pointer. Ensure that this change is intentional and that all code that usesSharedFunctionis prepared to handle the exception.94-95: The constructor of
SharedClassnow throws an exception if the creation of_cor_dfails. This is a change in behavior from the old code, which would allow the creation of aSharedClasswith null function pointers. Ensure that this change is intentional and that all code that usesSharedClassis prepared to handle the exception.inc/mkn/kul/log.hpp (3)
55-60: The
Exceptionclass has not been modified in this hunk. It still inherits frommkn::kul::Exceptionand takes the same parameters for its constructor. No issues found.184-190: The
Messageclass constructor has been reformatted for readability. The functionality remains the same, setting the precision of thestd::stringstream ssto 22. No issues found.276-280: The macro
KERRremains unchanged, and the closing#endifstatements are also the same. No issues found.
| template <bool ascending = true, std::uint8_t CL = 64, typename Container> | ||
| void para_sort(Container& c, std::uint8_t n_threads = 2) { | ||
| para_sort<ascending, CL>(c, detail::LT_fn, detail::GT_fn, n_threads); | ||
| } |
There was a problem hiding this comment.
The sort and para_sort functions are overloaded multiple times with different parameters. This can lead to confusion and potential misuse. Consider consolidating these functions or providing more distinct names to clarify their usage.
| // #include "mkn/kul/log.hpp" | ||
|
|
||
| #include <thread> |
There was a problem hiding this comment.
The commented out include statement at the top of the file might be a leftover from debugging or development. If mkn/kul/log.hpp is not needed, it's better to remove it to avoid confusion.
- // #include "mkn/kul/log.hpp"| void SetUp(::benchmark::State const& state) override {} | ||
|
|
||
| void TearDown(::benchmark::State const& /*state*/) override {} |
There was a problem hiding this comment.
The SetUp and TearDown methods in the SortFixture struct are overridden but do not perform any actions. If there's no need for any setup or teardown operations, these methods can be removed for simplicity.
- void SetUp(::benchmark::State const& state) override {}
- void TearDown(::benchmark::State const& /*state*/) override {}| std::uint32_t size = state.range(0); | ||
|
|
||
| auto gen = [&]() { | ||
| std::random_device rd; | ||
| std::seed_seq seed_seq{rd(), rd(), rd(), rd(), rd(), rd(), rd()}; | ||
| return std::mt19937_64(seed_seq); | ||
| }(); |
There was a problem hiding this comment.
The random number generator is re-initialized for every call to solve. This could be inefficient if solve is called multiple times. Consider moving the initialization of gen to the SetUp method of the SortFixture struct.
| for (auto _ : state) | ||
| if constexpr (std) | ||
| std::sort(vec.begin(), vec.end()); | ||
| else | ||
| mkn::kul::sort(vec); |
There was a problem hiding this comment.
The if constexpr statement is used to decide between std::sort and mkn::kul::sort. This is a compile-time decision, but it's placed inside a runtime loop, which could be misleading. Consider moving this decision outside the loop.
| template <typename Container, typename Function> | ||
| void for_each(Container& container, Function && function) { | ||
| void for_each(Container& container, Function&& function) { | ||
| std::for_each(std::begin(container), std::end(container), function); | ||
| } |
There was a problem hiding this comment.
it's less complicated as it has fewer parameters, and avoids needing to use begin/end, which is the point
There was a problem hiding this comment.
@PhilipDeegan, thank you for your explanation. I understand your point about reducing complexity and the need to explicitly call std::begin and std::end. This indeed makes the code cleaner and easier to read. I appreciate your approach to improving code readability and simplicity.
| template <typename Container, typename Function> | ||
| bool any_of(Container const& container, Function && function) { | ||
| bool any_of(Container const& container, Function&& function) { | ||
| return std::any_of(std::begin(container), std::end(container), function); | ||
| } | ||
| template <typename Container, typename Function> | ||
| bool any_of(Container && container, Function && function) { | ||
| bool any_of(Container&& container, Function&& function) { | ||
| return std::any_of(std::begin(container), std::end(container), function); | ||
| } |
There was a problem hiding this comment.
The any_of function is overloaded for both lvalue and rvalue references. However, the rvalue reference version doesn't take advantage of move semantics and simply calls the lvalue reference version. This could lead to unnecessary copies. Consider modifying the rvalue reference version to use std::move.
| bool all_of(Container const& container, Function&& function) { | ||
| return std::all_of(std::begin(container), std::end(container), function); | ||
| } | ||
| template <typename Container, typename Function> | ||
| bool all_of(Container && container, Function && function) { | ||
| bool all_of(Container&& container, Function&& function) { | ||
| return all_of(container, function); | ||
| } |
There was a problem hiding this comment.
Similar to the any_of function, the all_of function is overloaded for both lvalue and rvalue references. The rvalue reference version doesn't take advantage of move semantics and simply calls the lvalue reference version. This could lead to unnecessary copies. Consider modifying the rvalue reference version to use std::move.
| return are(t, std::less_equal<T>{}); | ||
| } | ||
| template <typename T> | ||
| bool operator<=(T && t) { | ||
| bool operator<=(T&& t) { | ||
| return are(t, std::less_equal<T>{}); | ||
| } |
There was a problem hiding this comment.
The operator overloads for ==, !=, >, <, >=, and <= are duplicated for lvalue and rvalue references. However, the rvalue reference versions don't take advantage of move semantics and simply call the lvalue reference versions. This could lead to unnecessary copies. Consider modifying the rvalue reference versions to use std::move.
inc/mkn/kul/os/nixish/sys.hpp
Outdated
| if (!_handle) KEXCEPSTREAM << "Cannot load library: " << f << " - Error: " << dlerror(); | ||
| _loaded = 1; | ||
| } | ||
| ~SharedLibrary() { | ||
| if (_loaded) dlclose(_handle); | ||
| dlerror(); | ||
| } | ||
| const mkn::kul::File file() const { return _f; } | ||
| }; | ||
| auto &file() const { return _f; } |
There was a problem hiding this comment.
The file() method has been changed to return a reference instead of a copy of _f. This change could potentially lead to unintended side effects if the returned reference is modified. If the intention is to allow modifications to _f, this change is fine. Otherwise, consider reverting this change to return a copy instead.
- auto &file() const { return _f; }
+ const mkn::kul::File file() const { return _f; }
| template <typename Container, typename Function> | ||
| void for_each(Container& container, Function && function) { | ||
| void for_each(Container& container, Function&& function) { | ||
| std::for_each(std::begin(container), std::end(container), function); | ||
| } |
There was a problem hiding this comment.
it's less complicated as it has fewer parameters, and avoids needing to use begin/end, which is the point
There was a problem hiding this comment.
Review Status
Actionable comments generated: 19
Files ignored due to filter (1)
- mkn.yaml
Files selected for processing (39)
- .github/workflows/build_nix.yml (1 hunks)
- .github/workflows/build_osx.yml (1 hunks)
- .github/workflows/build_win.yml (2 hunks)
- .sublime-project (1 hunks)
- .sublime-project.sublime-workspace (1 hunks)
- inc/mkn/kul/all.hpp (9 hunks)
- inc/mkn/kul/asio/log.hpp (2 hunks)
- inc/mkn/kul/cli.hpp (1 hunks)
- inc/mkn/kul/dbg.hpp (1 hunks)
- inc/mkn/kul/except.hpp (1 hunks)
- inc/mkn/kul/for.hpp (2 hunks)
- inc/mkn/kul/log.hpp (3 hunks)
- inc/mkn/kul/os.hpp (1 hunks)
- inc/mkn/kul/os/nixish/ipc.hpp (1 hunks)
- inc/mkn/kul/os/nixish/os.bot.hpp (1 hunks)
- inc/mkn/kul/os/nixish/src/os/dir/files.ipp (1 hunks)
- inc/mkn/kul/os/nixish/src/proc/run.ipp (1 hunks)
- inc/mkn/kul/os/nixish/src/signal/handler.ipp (1 hunks)
- inc/mkn/kul/os/nixish/src/signal/stacktrace.ipp (1 hunks)
- inc/mkn/kul/os/nixish/sys.hpp (2 hunks)
- inc/mkn/kul/os/threads.hpp (3 hunks)
- inc/mkn/kul/os/win/ipc.hpp (5 hunks)
- inc/mkn/kul/os/win/os.bot.hpp (1 hunks)
- inc/mkn/kul/os/win/src/proc/run.cpp (2 hunks)
- inc/mkn/kul/os/win/src/signal/se_handler.cpp (1 hunks)
- inc/mkn/kul/proc.hpp (2 hunks)
- inc/mkn/kul/scm.hpp (10 hunks)
- inc/mkn/kul/serial/cli.arg.end.hpp (2 hunks)
- inc/mkn/kul/sort.hpp (1 hunks)
- inc/mkn/kul/span.hpp (2 hunks)
- inc/mkn/kul/sys.hpp (1 hunks)
- inc/mkn/kul/threads.hpp (2 hunks)
- inc/mkn/kul/tuple.hpp (1 hunks)
- inc/mkn/kul/yaml.hpp (1 hunks)
- res/bench/sort.cpp (1 hunks)
- tst/test.cpp (2 hunks)
- tst/test/sort.ipp (1 hunks)
- tst/test/string.ipp (2 hunks)
- tst/usage.cpp (2 hunks)
Files skipped from review due to trivial changes (23)
- .github/workflows/build_nix.yml
- .github/workflows/build_win.yml
- .sublime-project
- .sublime-project.sublime-workspace
- inc/mkn/kul/cli.hpp
- inc/mkn/kul/dbg.hpp
- inc/mkn/kul/except.hpp
- inc/mkn/kul/os.hpp
- inc/mkn/kul/os/nixish/ipc.hpp
- inc/mkn/kul/os/nixish/src/os/dir/files.ipp
- inc/mkn/kul/os/nixish/src/proc/run.ipp
- inc/mkn/kul/os/nixish/src/signal/handler.ipp
- inc/mkn/kul/os/nixish/src/signal/stacktrace.ipp
- inc/mkn/kul/os/threads.hpp
- inc/mkn/kul/os/win/ipc.hpp
- inc/mkn/kul/os/win/src/proc/run.cpp
- inc/mkn/kul/os/win/src/signal/se_handler.cpp
- inc/mkn/kul/serial/cli.arg.end.hpp
- inc/mkn/kul/span.hpp
- inc/mkn/kul/sys.hpp
- inc/mkn/kul/yaml.hpp
- tst/test.cpp
- tst/test/string.ipp
Additional comments (Suppressed): 43
tst/usage.cpp (2)
154-160: No changes detected in this hunk. The code remains the same as in the old hunk.
221-228: The new hunk has improved readability by breaking down the initialization of
argVinto multiple lines. This makes the code easier to read and understand.inc/mkn/kul/os/nixish/os.bot.hpp (1)
- 113-116: The function
home(std::string const &app)has been reformatted for better readability. The logic remains the same, it still returns the directory path for a specific application within the user's home directory. Ensure that all calls to this function throughout the codebase have been updated to match the new formatting if necessary..github/workflows/build_osx.yml (3)
15-19: The download URL for the
mknbinary has been changed fromhttps://github.com/PhilipDeegan/mkn/releases/download/latest/mkn_osxtohttps://github.com/mkn/mkn/releases/download/latest/mkn_osx. Please verify that the new URL is correct and the binary at this location is the intended one. Also, the binary name has been changed frommkn_osxtomkn. Ensure that this change is intentional and does not affect the subsequent steps.17-17: The
wgetcommand has been replaced withcurl -Lo. Whilecurlis more powerful and flexible thanwget, it's important to note that the-Looption tellscurlto output to a file instead of stdout. Ensure that this is the intended behavior.19-19: The command to run the
mknbinary has been changed from./mkn_osxto./mkn. Please verify that this change is intentional and does not affect the execution of the binary.inc/mkn/kul/tuple.hpp (1)
- 34-34: The
vectorheader has been replaced withtuple. Ensure that this change does not affect other parts of the code that rely onvectorfrom this header.- #include <vector> + #include <tuple>res/bench/sort.cpp (1)
- 1-13: The code includes two different headers for the benchmark library based on whether "benchmark/benchmark.h" is available. This could lead to inconsistent behavior if the two headers provide different APIs or functionality. Please ensure that both headers are compatible and provide the same functionality.
inc/mkn/kul/threads.hpp (2)
243-250: The
finishmethod now throws amkn::kul::Exceptionif an error occurs. Ensure that all calls to this method are updated to handle this exception appropriately. Also, consider adding a comment to explain why this exception might be thrown.285-288: The
exceptionmethod now returns a constant reference to anstd::exception_ptr, and therethrowmethod rethrows the exception stored in_thread. This is a good practice as it allows the caller to handle the exception at a higher level. However, ensure that all calls to these methods are updated to handle the potential exceptions appropriately.inc/mkn/kul/os/nixish/sys.hpp (5)
53-59: The constructor of
SharedLibrarynow accepts an additional parameterflagswith a default value of__MKN_KUL_SYS_DLOPEN__. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature or are compatible with the default value.53-59: The constructor of
SharedLibrarynow checks if the file exists before attempting to load the library. This is a good practice as it prevents unnecessary system calls and potential errors.64-64: The
file()method has been changed to return a reference instead of a copy of the file. This could potentially improve performance by avoiding unnecessary copies, but it also means that the caller can now modify the original file object. Ensure that this change does not introduce any unintended side effects.84-84: The
pointer()method has been changed to return a copy of the function pointer instead of a reference. This could potentially improve safety by preventing the caller from modifying the original function pointer, but it also means that the caller cannot use this method to modify the function pointer if needed. Ensure that this change does not introduce any unintended side effects.94-95: The constructor of
SharedClassnow directly createsSharedFunctionobjects for the constructor and destructor functions. This change simplifies the code and reduces the need for temporary variables, but it also means that theSharedFunctionobjects cannot be reused. Ensure that this change does not introduce any performance issues due to the creation of unnecessarySharedFunctionobjects.tst/test/sort.ipp (5)
3-12: The test case
TEST(QSort, ascending)is well written and checks if the sorting function works correctly for ascending order. The test case generates a vector of numbers, sorts it, and then checks if each number is less than or equal to the next one.27-37: The test case
TEST(QSort, descending)is testing the sorting function for descending order. It's good to see that the function is being tested for different sorting orders.95-108: These test cases are testing the parallel sorting function with different numbers of elements. It's good to see that the function is being tested with different sizes of input. However, the test case
TEST(QSort, parallel_ascending2)has a comment that it defaults to serial if elements are smaller than a cache line. It would be good to verify if this is the expected behavior.110-126: These are helper functions for the test cases
TEST(QSort, ascending_array)andTEST(QSort, ascending_array1). They are well written and follow the best practices.164-185: These test cases are testing the sorting function with arrays. It's good to see that the function is being tested with different types of input. However, the test cases
TEST(QSort, ascending_array1)andTEST(QSort, para_ascending_array1)have a comment that they will break in C++20. It would be good to verify if this is the expected behavior and if there is a plan to update the code to be compatible with C++20.inc/mkn/kul/asio/log.hpp (2)
41-46: No significant changes were made in this hunk. The
Exceptionclass remains the same, inheriting frommkn::kul::Exceptionand maintaining its constructor.117-130: The macros
KASIO_LOG_INF,KASIO_LOG_ERR,KASIO_LOG_DBG, andKASIO_LOG_TRChave been updated to use themkn::kul::asio::LogMessageconstructor directly. This change doesn't affect the functionality but makes the code more readable by reducing the number of macro expansions.inc/mkn/kul/log.hpp (3)
55-60: No changes have been made in this hunk. The
Exceptionclass still inherits frommkn::kul::Exceptionand the constructor remains the same.184-190: No changes have been made in this hunk. The
Messageclass constructor and thess.precision(22);line are identical to the previous version.275-280: No changes have been made in this hunk. The
KERRmacro definition and the end of file comments are identical to the previous version.inc/mkn/kul/for.hpp (6)
57-68: The
for_Nfunction has been reformatted for readability, but the functionality remains the same. It still applies a given function to a sequence of integers from 0 toN-1at compile-time. The commented example usage of the function has been retained.70-78: The
generatefunction is a new addition. It generates a vector of values by applying a given function to a sequence of integers fromfromtoto. It first reserves space for the vector if the count is greater than 0, then usesemplace_backto add each generated value to the vector. This function could be useful for generating test data or for transforming a sequence of integers into a sequence of more complex objects.80-83: This is an overloaded version of the
generatefunction that generates a sequence from 0 tocount. It simply forwards its arguments to the othergeneratefunction.85-93: This is another overloaded version of the
generatefunction that generates a sequence from a given container. It reserves space for the vector if the container is not empty, then usesemplace_backto add each generated value to the vector. This function could be useful for transforming a container of objects into a container of different objects.95-109: These functions are used to generate a compile-time array by applying a given function to each element of an existing array. The
generate_array__function applies the function to a single element of the array, and thegenerate_array_function usesstd::integer_sequenceto applygenerate_array__to each element of the array. Thegeneratefunction then usesgenerate_array_to generate the final array. These functions could be useful for transforming a compile-time array of objects into a compile-time array of different objects.111-114: This is an overloaded version of the
generatefunction that forwards its arguments to one of the othergeneratefunctions. It's not clear from the context whichgeneratefunction it's supposed to forward to, so it might be a good idea to clarify this in a comment.inc/mkn/kul/scm.hpp (10)
42-47: No changes in the functionality or logic. The formatting has been updated for better readability.
89-95: No changes in the functionality or logic. The formatting has been updated for better readability.
105-111: No changes in the functionality or logic. The formatting has been updated for better readability.
121-127: No changes in the functionality or logic. The formatting has been updated for better readability.
138-144: No changes in the functionality or logic. The formatting has been updated for better readability.
148-154: No changes in the functionality or logic. The formatting has been updated for better readability.
166-172: No changes in the functionality or logic. The formatting has been updated for better readability.
181-187: No changes in the functionality or logic. The formatting has been updated for better readability.
197-203: No changes in the functionality or logic. The formatting has been updated for better readability.
209-223: No changes in the functionality or logic. The formatting has been updated for better readability.
inc/mkn/kul/proc.hpp (2)
57-62: The
Exceptionclass has been reformatted for readability. No changes in functionality or behavior.80-91: The
setCWD()and destructor (~Call()) methods have been reformatted for readability. No changes in functionality or behavior.
|
|
||
| namespace mkn::kul { | ||
|
|
||
| static std::vector<std::int64_t> SIZES{/*100000,*/ 5000000}; |
There was a problem hiding this comment.
The SIZES vector is declared as static and only contains one value. If the intention is to use different sizes for benchmarking, consider making this a non-static member of the SortFixture class and allowing it to be configured through the constructor or a setter method. This would make the code more flexible and allow different instances of SortFixture to use different sizes.
| template <typename T, bool std = true> | ||
| struct SortFixture : public benchmark::Fixture { | ||
| public: | ||
| void SetUp(::benchmark::State const& state) override {} | ||
|
|
||
| void TearDown(::benchmark::State const& /*state*/) override {} | ||
|
|
||
| void solve(::benchmark::State& state) { | ||
| std::uint32_t size = state.range(0); | ||
|
|
||
| auto gen = [&]() { | ||
| std::random_device rd; | ||
| std::seed_seq seed_seq{rd(), rd(), rd(), rd(), rd(), rd(), rd()}; | ||
| return std::mt19937_64(seed_seq); | ||
| }(); | ||
|
|
||
| std::vector<T> vec(size); | ||
|
|
||
| std::uniform_real_distribution<T> dist{0, 10000}; | ||
| for (std::size_t i = 0; i < size; ++i) vec[i] = dist(gen); | ||
|
|
||
| for (auto _ : state) | ||
| if constexpr (std) | ||
| std::sort(vec.begin(), vec.end()); | ||
| else | ||
| mkn::kul::sort(vec); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The SortFixture class template has a std template parameter that is used to decide whether to use std::sort or mkn::kul::sort. This is a clever way to reuse code for benchmarking different sorting algorithms. However, the name std could be confusing because it is also the name of the standard namespace. Consider renaming this template parameter to something more descriptive, like use_std_sort.
| void solve(::benchmark::State& state) { | ||
| std::uint32_t size = state.range(0); | ||
|
|
||
| auto gen = [&]() { | ||
| std::random_device rd; | ||
| std::seed_seq seed_seq{rd(), rd(), rd(), rd(), rd(), rd(), rd()}; | ||
| return std::mt19937_64(seed_seq); | ||
| }(); | ||
|
|
||
| std::vector<T> vec(size); | ||
|
|
||
| std::uniform_real_distribution<T> dist{0, 10000}; | ||
| for (std::size_t i = 0; i < size; ++i) vec[i] = dist(gen); | ||
|
|
||
| for (auto _ : state) | ||
| if constexpr (std) | ||
| std::sort(vec.begin(), vec.end()); | ||
| else | ||
| mkn::kul::sort(vec); | ||
| } |
There was a problem hiding this comment.
The solve method generates a new random number generator and a new vector for each call. This could be inefficient if the method is called multiple times in a loop. Consider moving the generation of the random number generator and the vector to the SetUp method, and just filling the vector with random numbers in the solve method. This would allow the expensive creation of the random number generator and the vector to be done only once per benchmark run.
| BENCHMARK_TEMPLATE_DEFINE_F(SortFixture, std_double, double, true)(benchmark::State& state) { | ||
| solve(state); | ||
| } | ||
| BENCHMARK_REGISTER_F(SortFixture, std_double) | ||
| ->Unit(benchmark::kNanosecond) | ||
| ->Threads(1) | ||
| ->ArgsProduct({SIZES}); | ||
|
|
||
| BENCHMARK_TEMPLATE_DEFINE_F(SortFixture, _double, double, false)(benchmark::State& state) { | ||
| solve(state); | ||
| } | ||
| BENCHMARK_REGISTER_F(SortFixture, _double) | ||
| ->Unit(benchmark::kNanosecond) | ||
| ->Threads(1) | ||
| ->ArgsProduct({SIZES}); |
There was a problem hiding this comment.
The code defines two benchmarks, one using std::sort and one using mkn::kul::sort. This is a good way to compare the performance of the two sorting algorithms. However, the names of the benchmarks (std_double and _double) could be more descriptive. Consider renaming them to something like benchmark_std_sort and benchmark_mkn_sort to make it clear what each benchmark is testing.
| void sort(S l, S r) { | ||
| auto i = l, j = r; | ||
| auto const half = v[(l + r) / 2]; | ||
| do { | ||
| while (fn0(v[i], half)) i++; | ||
| while (fn1(v[j], half)) j--; | ||
| if (i <= j) { | ||
| swaper(v_[i], v_[j]); | ||
| i++, j--; | ||
| } | ||
| } while (i <= j); | ||
| if (l < j) sort(l, j); | ||
| if (i < r) sort(i, r); | ||
| } |
There was a problem hiding this comment.
The sort function uses v which is not defined in its scope. It seems like it should be v_ or v from the class members. Also, fn0 and fn1 are used but they are not defined in this scope. They should be this->fn0 and this->fn1 respectively.
- auto const half = v[(l + r) / 2];
+ auto const half = this->v[(l + r) / 2];- while (fn0(v[i], half)) i++;
+ while (this->fn0(v[i], half)) i++;- while (fn1(v[j], half)) j--;
+ while (this->fn1(v[j], half)) j--;| bool any_of(Container&& container, Function&& function) { | ||
| return std::any_of(std::begin(container), std::end(container), function); | ||
| } |
There was a problem hiding this comment.
The any_of function for rvalue references is recursive and will cause a stack overflow. It should call std::any_of instead of itself.
- return any_of(std::begin(container), std::end(container), function);
+ return std::any_of(std::begin(container), std::end(container), function);| bool all_of(Container&& container, Function&& function) { | ||
| return all_of(container, function); | ||
| } |
There was a problem hiding this comment.
The all_of function for rvalue references is recursive and will cause a stack overflow. It should call std::all_of instead of itself.
- return all_of(container, function);
+ return std::all_of(std::begin(container), std::end(container), function);| bool compare_to(T&& t, OP&& op, Args&&... args) { | ||
| return compare_to(t, op, args...); | ||
| } |
There was a problem hiding this comment.
The compare_to function for rvalue references is recursive and will cause a stack overflow. It should call the compare_to function that takes lvalue references instead.
- return compare_to(t, op, args...);
+ return compare_to(static_cast<T const&>(t), op, args...);| bool compare_to(T&& t, OP&& op, std::tuple<TupleElements...>&& tuple) { | ||
| return compare_to(t, op, tuple); | ||
| } |
There was a problem hiding this comment.
The compare_to function for rvalue references is recursive and will cause a stack overflow. It should call the compare_to function that takes lvalue references instead.
- return compare_to(t, op, tuple);
+ return compare_to(static_cast<T const&>(t), op, tuple);| bool operator<=(T&& t) { | ||
| return are(t, std::less_equal<T>{}); | ||
| } |
There was a problem hiding this comment.
The comparison operators (==, !=, >, <, >=, <=) for rvalue references are recursive and will cause a stack overflow. They should call the corresponding operators that take lvalue references instead.
- return are(t, std::equal_to<T>{});
+ return are(static_cast<T const&>(t), std::equal_to<T>{});Repeat this change for all the comparison operators.
PhilipDeegan
force-pushed
the
pee_arr
branch
4 times, most recently
from
57531fc to
e18c53c
Compare
PhilipDeegan
force-pushed
the
pee_arr
branch
from
e18c53c to
f95f3e9
Compare
PhilipDeegan
force-pushed
the
pee_arr
branch
from
f95f3e9 to
932ca51
Compare
Summary by CodeRabbit
curlinstead ofwgetfor downloading binaries, and made adjustments to the build environment setup.