Skip to content

RAII for xr core funcs struct#19

Merged
aristarkhovNV merged 1 commit intomainfrom
aaristarkhov/core_funcs
Dec 18, 2025
Merged

RAII for xr core funcs struct#19
aristarkhovNV merged 1 commit intomainfrom
aaristarkhov/core_funcs

Conversation

@aristarkhovNV
Copy link
Collaborator

No description provided.

@aristarkhovNV aristarkhovNV self-assigned this Dec 18, 2025
Copilot AI review requested due to automatic review settings December 18, 2025 02:25
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 refactors the OpenXRCoreFunctions struct to use RAII principles by converting it from a mutable object with a load() method to an immutable value type created via a static factory method. This eliminates manual initialization checks and enables safer usage patterns.

Key Changes:

  • Converted OpenXRCoreFunctions::load() from an instance method returning bool to a static factory method returning the struct by value
  • Made core_funcs_ member variables const and initialized them in constructor initializer lists
  • Removed explicit default constructor and manual error handling at call sites

Reviewed changes

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

Show a summary per file
File Description
src/core/oxr_utils/cpp/inc/oxr_utils/oxr_funcs.hpp Converted load() to static factory method, removed default constructor, added exception on failure
src/core/xrio/cpp/inc/xrio/headtracker.hpp Made core_funcs_ const and moved to top of member list
src/core/xrio/cpp/inc/xrio/controllertracker.hpp Made core_funcs_ const and moved to top of member list
src/core/xrio/cpp/headtracker.cpp Initialize core_funcs_ in constructor initializer list using static load()
src/core/xrio/cpp/controllertracker.cpp Initialize core_funcs_ in constructor initializer list using static load()
src/core/xrio/cpp/handtracker.cpp Use static load() to create local core_funcs variable

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

{
return false;
}
assert(getProcAddr);
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The assert will only trigger in debug builds. Consider using an explicit runtime check with a clear error message (e.g., if (!getProcAddr) throw std::invalid_argument("getProcAddr cannot be null");) to ensure consistent behavior across build configurations.

Suggested change
assert(getProcAddr);
if (!getProcAddr)
{
throw std::invalid_argument("getProcAddr cannot be null");
}

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is not supposed to happen in production, and is not to be handled by the caller.

}
assert(getProcAddr);

OpenXRCoreFunctions results{};
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The struct members are not default-initialized in the new design. Without an explicit constructor or member initializers, the function pointers will have indeterminate values. Add = nullptr initializers to each member declaration or define a default constructor that initializes all members to nullptr.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

{} will initialize all members to zero

Copy link
Collaborator

@jiwenc-nv jiwenc-nv left a comment

Choose a reason for hiding this comment

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

Please address the CoPilot review as well.

I don't have strong opinion on whether assert is sufficient. What @aristarkhovNV had in the original PR also LGTM.

@aristarkhovNV
Copy link
Collaborator Author

Please address the CoPilot review as well.

I don't have strong opinion on whether assert is sufficient. What @aristarkhovNV had in the original PR also LGTM.

It is a program logic consistency self-check. Asserted condition is never supposed to happen in production and is not intended to be handled by the caller.

@aristarkhovNV aristarkhovNV merged commit 1d05ada into main Dec 18, 2025
4 checks passed
@aristarkhovNV aristarkhovNV deleted the aaristarkhov/core_funcs branch December 18, 2025 22:09
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.

3 participants