Skip to content

Conversation

@Crazy-Rich-Meghan
Copy link
Contributor

  1. Changed m_points and m_data in gsLookupFunctionSingle to hold references instead of copies, reducing unnecessary data duplication.
  2. Changed the Container typedef in gsLookupFunction to use memory::unique_ptr for storing Piece_t objects
  3. Update pointer by using reset()

@Crazy-Rich-Meghan Crazy-Rich-Meghan added this to the v25.06.0 milestone Jun 27, 2025
@Crazy-Rich-Meghan Crazy-Rich-Meghan self-assigned this Jun 27, 2025
@Crazy-Rich-Meghan Crazy-Rich-Meghan added the enhancement New feature or request label Jun 27, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors gsLookupFunction to use unique_ptr for Piece_t storage, updates cloning to handle pointer semantics, and changes gsLookupFunctionSingle to hold references for points and data.

  • Switched Container to std::vector<memory::unique_ptr<Piece_t>>
  • Implemented deep-copy clone() via clone_impl()
  • Updated add(), set(), and evaluation methods to work with pointers
  • Changed m_points and m_data in gsLookupFunctionSingle to references
Comments suppressed due to low confidence (2)

gsLookupFunction.h:158

  • [nitpick] The comment references shared_ptr even though it isn’t used elsewhere. Clarify why unique_ptr is chosen (e.g., ownership semantics) or remove the shared_ptr mention.
        // Use unique_ptr - more efficient than shared_ptr

gsLookupFunction.h:326

  • [nitpick] Holding m_points as a reference risks dangling references if the original matrices go out of scope. Consider using an owning type (shared_ptr/unique_ptr) or ensure callers manage the source lifetime.
    const gsMatrix<T>& m_points;

@Crazy-Rich-Meghan Crazy-Rich-Meghan merged commit efb3752 into main Jul 11, 2025
1 check failed
@Crazy-Rich-Meghan Crazy-Rich-Meghan deleted the hot-fix-lookup branch July 11, 2025 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants