Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix build for gcc 15 #3389

Open
wants to merge 1 commit into
base: nat/poc_cpp23
Choose a base branch
from

Conversation

AiraYumi
Copy link
Contributor

@AiraYumi AiraYumi commented Jan 9, 2025

Fix build error for gcc 15

@github-actions github-actions bot added the c/cpp label Jan 9, 2025
@AiraYumi AiraYumi changed the title fix maybe uninitialized for gcc 15 fix build for gcc 15 Jan 10, 2025
@@ -310,7 +310,7 @@ const LLVoiceVersionInfo LLVoiceClient::getVersion()
}
else
{
LLVoiceVersionInfo result;
LLVoiceVersionInfo result = {};
result.serverVersion = std::string();
result.voiceServerType = std::string();
result.mBuildVersion = std::string();
Copy link
Collaborator

Choose a reason for hiding this comment

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

If serverVersion, voiceServerType and mBuildVersion are std::string members of LLVoiceVersionInfo, wouldn't the new initialization at line 313 make lines 314-316 redundant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll check.
There's a good chance that lines 314 to 316 can be deleted.

Copy link
Contributor Author

@AiraYumi AiraYumi Jan 10, 2025

Choose a reason for hiding this comment

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

I found that lines 314 to 316 could be deleted.

When defining a variable, we can also use imperative initialization, but which is better?

e.g.

LLVoiceVersionInfo result{
    .voiceServerType = std::string()
    , .internalVoiceServerType = std::string()
    , .majorVersion = 0
    , .minorVersion = 0
    , .serverVersion = std::string()
    , .mBuildVersion = std::string()
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

Designated initialization would be great if any one of those members had a non-default value. But to be honest, at least from my perspective,

LLVoiceVersionInfo result{};

is clearer than the designated initialization you wrote above. I think we all understand that value-initializing std::string produces an empty string, and value-initializing an int produces 0. If, in reading the code, I hit that 8-line initialization expression, I must consider it to understand why it's written it out that way. Is any one of those members being initialized to a non-default value? No? Are we intentionally omitting mention of any members, maybe because they're declared with their own initial values in the struct definition? That sends me looking for the struct definition.

If I see a one-line initialization to {}, I can nod and understand that all members are initialized to their default values, and go on.

@AiraYumi AiraYumi marked this pull request as ready for review January 10, 2025 22:08
@AiraYumi
Copy link
Contributor Author

all fixed

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