Skip to content

Conversation

@yhabteab
Copy link
Member

No description provided.

@yhabteab yhabteab added the core/quality Improve code, libraries, algorithms, inline docs label Feb 14, 2023
@yhabteab yhabteab requested a review from Al2Klimov February 14, 2023 08:45
@cla-bot cla-bot bot added the cla/signed label Feb 14, 2023
@Al2Klimov Al2Klimov added this to the 2.14.0 milestone Feb 14, 2023
Copy link
Member

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

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

(I haven't looked into the other changes in detail, just the one where I know there is a problem.)

Comment on lines 111 to 118
for (int i = 0; i < envc; i++) {
if (strncmp(environ[i], lcnumeric, strlen(lcnumeric)) == 0) {
if (strcmp(environ[i], lcnumeric) == 0) {
continue;
}

if (strncmp(environ[i], notifySocket, strlen(notifySocket)) == 0) {
if (strcmp(environ[i], notifySocket) == 0) {
continue;
}
Copy link
Member

Choose a reason for hiding this comment

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

That's actually not just a redundant overflow protection but is actually implementing a prefix check here. But that's totally not obvious here, I also got this wrong first. So changing the code to make this more obvious would actually be a quality improvement.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, just remove f5380cf.

@julianbrost julianbrost requested review from Al2Klimov and removed request for Al2Klimov March 10, 2023 15:11
@Al2Klimov Al2Klimov marked this pull request as draft March 20, 2023 10:43
@lippserd lippserd removed this from the 2.14.0 milestone Mar 30, 2023
@Al2Klimov Al2Klimov self-assigned this Jun 1, 2023
@yhabteab yhabteab closed this Jan 26, 2026
@yhabteab yhabteab deleted the code-optimization-2 branch January 26, 2026 14:16
@Al2Klimov Al2Klimov restored the code-optimization-2 branch January 26, 2026 14:33
@Al2Klimov Al2Klimov reopened this Jan 26, 2026
Comment on lines 111 to 118
for (int i = 0; i < envc; i++) {
if (strncmp(environ[i], lcnumeric, strlen(lcnumeric)) == 0) {
if (strcmp(environ[i], lcnumeric) == 0) {
continue;
}

if (strncmp(environ[i], notifySocket, strlen(notifySocket)) == 0) {
if (strcmp(environ[i], notifySocket) == 0) {
continue;
}
Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, just remove f5380cf.

@yhabteab
Copy link
Member Author

All the changes from this PR can be found in #9502, so there's no need for this.

@yhabteab yhabteab closed this Jan 26, 2026
@yhabteab yhabteab deleted the code-optimization-2 branch January 26, 2026 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla/signed core/quality Improve code, libraries, algorithms, inline docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants