Skip to content

Conversation

@brbzull0
Copy link
Contributor

@brbzull0 brbzull0 commented Oct 31, 2025

Didn't like how the cast was used in the caller, so now the cast is hidden in one place (implementation detail).

API changed but it's just internal .

@brbzull0 brbzull0 added this to the 10.2.0 milestone Oct 31, 2025
@brbzull0 brbzull0 self-assigned this Oct 31, 2025
@brbzull0 brbzull0 requested a review from Copilot October 31, 2025 12:46
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 improves the type safety of the RecSetRecordString function by changing its parameter type from const RecString to RecStringConst (both are const char * but using the more semantically appropriate type alias).

  • Changed function signature to use RecStringConst instead of const RecString for better type clarity
  • Moved the const_cast from caller code into the implementation
  • Simplified caller code by removing the need for explicit const_cast

Reviewed Changes

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

File Description
include/records/RecCore.h Updated function declaration to use RecStringConst parameter type
src/records/P_RecCore.cc Updated function definition and added internal const_cast when assigning to union
src/mgmt/rpc/handlers/config/Configuration.cc Removed unnecessary const_cast from caller now that function accepts const strings directly

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

Copy link
Contributor

@cmcfarlen cmcfarlen left a comment

Choose a reason for hiding this comment

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

I've run into this annoyance as well. Thanks for cleaning it up!

@brbzull0 brbzull0 merged commit d727f20 into apache:master Nov 3, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants