Skip to content

fix unwanted copies of return values - #185

Merged
Dimitris Gkanatsios (dgkanatsios) merged 3 commits into
PlayFab:mainfrom
maxbachmann:patch-1
Mar 6, 2026
Merged

fix unwanted copies of return values#185
Dimitris Gkanatsios (dgkanatsios) merged 3 commits into
PlayFab:mainfrom
maxbachmann:patch-1

Conversation

@maxbachmann

@maxbachmann Max Bachmann (maxbachmann) commented May 23, 2025

Copy link
Copy Markdown
Contributor

Update public API return types to avoid returning by const value. This improves usability since callers can modify or move the returned value.

In addition this avoids an unnecessary copy of the m_configSettings hashmap while under the config mutex

@maxbachmann Max Bachmann (maxbachmann) changed the title fix unwanted string copy fix unwanted copies of return values May 23, 2025
@maxbachmann

Max Bachmann (maxbachmann) commented May 23, 2025

Copy link
Copy Markdown
Contributor Author

Edit: Thinking about this a bit more this was probably supposed to guard against the config hashmap getting modified.

I cleaned this up in a couple of ways:

  • return by value instead of const value
  • remove outdated static empty string that doesn't serve any purpose anymore
  • avoid unnecessary copy of the config hashmap when getting the log / shared content directory

@maxbachmann
Max Bachmann (maxbachmann) marked this pull request as draft May 23, 2025 13:51
@maxbachmann
Max Bachmann (maxbachmann) marked this pull request as ready for review May 23, 2025 13:59
@maxbachmann

Copy link
Copy Markdown
Contributor Author

@maxbachmann

Copy link
Copy Markdown
Contributor Author

@dgkanatsios

Copy link
Copy Markdown
Contributor

Really sorry for the delay, team has been really busy. Promise to take a look as fast as we can!

@rgomez391

Copy link
Copy Markdown
Contributor

Max Bachmann (@maxbachmann) just one minor note: The PR description ("supposed to return a const reference") is slightly misleading — the fix actually makes them return non-const values by value, which is the correct approach.

@rgomez391

Copy link
Copy Markdown
Contributor

The code looks correct to me and I verified that there are no compilation issues.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Updates the C++ GSDK public API return types to avoid top-level const on return-by-value results, improving usability (callers can mutate the returned value) and enabling move semantics where applicable.

Changes:

  • Remove top-level const from return-by-value APIs (getConfigSettings, getLogsDirectory, getSharedContentDirectory) in the public header and implementation.
  • Avoid an extra copy of m_configSettings inside getLogsDirectory / getSharedContentDirectory by referencing the internal map under the config mutex.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
cpp/cppsdk/gsdk.h Updates public API signatures to remove top-level const on return-by-value types.
cpp/cppsdk/gsdk.cpp Matches updated signatures and reduces internal copying when reading config values.

Comment thread cpp/cppsdk/gsdk.h
@dgkanatsios

Copy link
Copy Markdown
Contributor

merging, thanks for the contribution Max Bachmann (@maxbachmann) !

@dgkanatsios
Dimitris Gkanatsios (dgkanatsios) merged commit d864af0 into PlayFab:main Mar 6, 2026
4 checks passed
Copilot AI pushed a commit that referenced this pull request Mar 19, 2026
* fix unwanted string copy

* fix more occurences and follow code formatting

* fix return of local variable
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.

4 participants