Skip to content

Conversation

@ralwing
Copy link

@ralwing ralwing commented Sep 1, 2025

Related Issues & PRs

ouster-lidar/ouster-ros#485

Summary of Changes

This change replaces C like extern variable linkage with getter function

Validation

Build and tests.

Related to ouster-lidar/ouster-ros#486

Copilot AI review requested due to automatic review settings September 1, 2025 06:30

This comment was marked as outdated.

@ralwing ralwing force-pushed the fix-double-free branch 2 times, most recently from a23aa16 to 4aa6a52 Compare September 1, 2025 07:38
@ralwing ralwing requested a review from Copilot September 1, 2025 07:39

This comment was marked as outdated.

Copy link

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 fixes a potential double-free issue by replacing C-style extern variable declarations with getter functions that return references to static variables. This change improves encapsulation and prevents potential memory management issues.

  • Replace extern variable declarations with getter function declarations in the header
  • Convert extern variable definitions to static variables with corresponding getter functions
  • Update callers to use getter functions instead of direct variable access

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
ouster_client/include/ouster/types.h Updates header declarations to use getter functions instead of extern variables
ouster_client/src/sensor_info.cpp Implements static variables with getter functions and updates usage sites

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@Samahu Samahu self-requested a review September 3, 2025 15:51
Copy link
Collaborator

@Samahu Samahu left a comment

Choose a reason for hiding this comment

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

looks good to me, fully agree with you findings.

Co-authored-by: Ussama Naal <606033+Samahu@users.noreply.github.com>
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