Skip to content

sapi/*: move duplicate "--define" code to library #8241

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

Closed
wants to merge 1 commit into from

Conversation

MaxKellermann
Copy link
Contributor

No description provided.

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

Other than the nit this looks good to me

@iluuu1994
Copy link
Member

Does anyone else want to review this? Otherwise I'll merge this tomorrow or so.

@iluuu1994
Copy link
Member

Sorry, forgot about this one. I'll merge it tomorrow.

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

Other than the small coding style remark, LGTM

Comment on lines +25 to +26
if (b->length > 0)
memmove(b->value + length, b->value, b->length);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use explicit {} for the if block?

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK we don't have any guidelines on this. I'm fine with making braces mandatory for new code but I think the risks of not adding braces are largely overblown.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's a risk, just confusing IMHO (at least I get confused)

Copy link
Member

Choose a reason for hiding this comment

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

The argument is usually somebody might extend the block in the if and forget that braces are missing.

 if (foo())
     bar();
+    baz();

Or bar() might be a macro containing multiple statements but we use do {} while (0) in our macros to prevent that.

Copy link
Member

Choose a reason for hiding this comment

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

@iluuu1994 just FYI we actually have guidelines on this:

php-src/CODING_STANDARDS.md

Lines 243 to 258 in 31692a1

3. Be generous with whitespace and braces. Keep one empty line between the
variable declaration section and the statements in a block, as well as
between logical statement groups in a block. Maintain at least one empty
line between two functions, preferably two. Always prefer:
```c
if (foo) {
bar;
}
```
to:
```c
if(foo)bar;
```

Copy link
Member

Choose a reason for hiding this comment

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

@bukka Ah, good to know.

@iluuu1994
Copy link
Member

Merged as d87ba95. Thanks @MaxKellermann!

@iluuu1994 iluuu1994 closed this Apr 18, 2022
@iluuu1994
Copy link
Member

004+ 
005+ =================================================================
004- PHP %s
005- Done: 0
006+ ==273596==ERROR: LeakSanitizer: detected memory leaks
007+ 
008+ Direct leak of 17 byte(s) in 1 object(s) allocated from:
009+     #0 0x7f4371883c3e in __interceptor_realloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:163
010+     #1 0x55ebfd0adf46 in php_ini_builder_realloc /home/runner/work/php-src/php-src/main/php_ini_builder.h:65
011+     #2 0x55ebfd0af61d in php_ini_builder_define /home/runner/work/php-src/php-src/main/php_ini_builder.c:73
012+     #3 0x55ebfdc52e80 in main /home/runner/work/php-src/php-src/sapi/fpm/fpm/fpm_main.c:1588
013+     #4 0x7f436cb230b2 in __libc_start_main (/usr/lib/x86_64-linux-gnu/libc.so.6+0x240b2)
014+ 
015+ SUMMARY: AddressSanitizer: 17 byte(s) leaked in 1 allocation(s).
016+ PHP 8.2.0-dev (fpm-fcgi) (built: Apr 19 2022 01:43:52) (DEBUG)
017+ Done: 1

There's a new leak. I'm at work right now but I'll have a look tonight.

iluuu1994 added a commit to iluuu1994/php-src that referenced this pull request Apr 19, 2022
Temporarily store result of ini bulder in ini_entries to avoid a leak
when main() exists prematurely. Technically ini_entries isn't released
either but ASAN doesn't consider unreleased memory referenced from
globals leaks.
iluuu1994 added a commit to iluuu1994/php-src that referenced this pull request Apr 19, 2022
Temporarily store result of ini builder in ini_entries to avoid a leak
when main() exists prematurely. Technically ini_entries isn't released
either but ASAN doesn't consider unreleased memory referenced from
globals leaks.
@MaxKellermann MaxKellermann deleted the php_ini_builder branch April 20, 2022 15:46
iluuu1994 added a commit that referenced this pull request Apr 25, 2022
Temporarily store result of ini builder in ini_entries to avoid a leak
when main() exists prematurely. Technically ini_entries isn't released
either but ASAN doesn't consider unreleased memory referenced from
globals leaks.
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