Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

more alloc - asan warnings #36

Merged
merged 1 commit into from
Nov 19, 2023
Merged

more alloc - asan warnings #36

merged 1 commit into from
Nov 19, 2023

Conversation

PhilipDeegan
Copy link
Member

@PhilipDeegan PhilipDeegan commented Sep 24, 2022

Summary by CodeRabbit

  • Refactor
    • Improved memory alignment handling in the allocation process to ensure better performance and stability.
  • New Features
    • Enhanced the Span class with new member functions for resetting and assigning from compatible containers, increasing flexibility in managing span-like data structures.

Copy link

coderabbitai bot commented Nov 19, 2023

Walkthrough

The recent updates involve enhancing memory alignment handling in the allocation header and extending the functionality of the Span class to accept and adapt to span-like containers. These changes improve the flexibility and usability of the library without altering its existing interface or behavior.

Changes

File Path Change Summary
.../kul/alloc.hpp Changed alignment type from std::size_t to std::int32_t and adjusted allocation size for proper alignment.
.../kul/span.hpp Added reset and operator= functions to Span class for compatibility with span-like containers.

🐇🍂 In the heart of fall, we tweak and we tune,
Code aligns just right, under the harvest moon.
Spans grow and adapt, like leaves to the breeze,
Our code hops along with the greatest of ease. 🌕✨


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between aa5093f and ce80e02.
Files selected for processing (2)
  • inc/mkn/kul/alloc.hpp (3 hunks)
  • inc/mkn/kul/span.hpp (1 hunks)
Additional comments: 3
inc/mkn/kul/alloc.hpp (3)
  • 39-42: Changing the template parameter alignment from std::size_t to std::int32_t could potentially introduce issues if the alignment is expected to be larger than what std::int32_t can hold. However, since the alignment is typically a small power of two, this is unlikely to be a problem in practice. Still, it's important to ensure that all uses of AlignedAllocator are checked to confirm that a std::int32_t is sufficient and that no negative values are being used for alignment, as that would be nonsensical and could lead to undefined behavior.

  • 54-64: The changes to the allocate function look correct and ensure that the allocated memory is properly aligned. The calculation of diff ensures that the allocated size is always a multiple of the alignment, which is important for alignment guarantees. The use of std::aligned_alloc is appropriate here, and the check for a null pointer before throwing std::bad_alloc is a good practice. However, it's important to note that std::aligned_alloc requires the size to be a multiple of the alignment, which is ensured by the added calculation. This change should be safe as long as the alignment value is a power of two, which is a common requirement for memory alignment.

  • 84-87: No issues with the closing of the namespace and the include guard. These are standard practices in C++ to prevent symbol redefinition and to ensure that the file is only included once during compilation.

Comment on lines 69 to 87
auto end() const { return ptr + s; }
SIZE const& size() const { return s; }

template <typename C, std::enable_if_t<is_span_like_v<C>, bool> = 0>
void reset(C& c) {
ptr = c.data();
s = c.size();
}

template <typename C, std::enable_if_t<is_span_like_v<C>, bool> = 0>
auto& operator=(C& c) {
reset(c);
return *this;
}


private:
T* ptr = nullptr;
SIZE s = 0;
Copy link

Choose a reason for hiding this comment

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

The implementation of the reset and operator= functions for the Span class is correct and follows the expected behavior of resetting the internal pointer and size to match the provided container. The use of std::enable_if_t with the is_span_like_v concept ensures that these functions are only available for types that have data() and size() member functions, which is a good use of SFINAE (Substitution Failure Is Not An Error) to enforce type constraints.

However, it's important to note that the size() method in line 75 should be cast to SIZE to ensure type consistency, especially if c.size() returns a type that is different from SIZE. This is to prevent any potential type narrowing or conversion issues.

Also, the reset and operator= functions do not check if the container c is empty before assigning its data pointer to ptr. If c is empty, c.data() might return nullptr, and ptr would also be set to nullptr. This behavior should be documented or handled if it's not the intended behavior.

@PhilipDeegan PhilipDeegan merged commit 51662eb into master Nov 19, 2023
@PhilipDeegan PhilipDeegan deleted the alloc branch November 19, 2023 12:35
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.

None yet

1 participant