-
Notifications
You must be signed in to change notification settings - Fork 7.9k
zend_ast: Mark zend_ast_list_add()
as ZEND_ATTRIBUTE_NODISCARD
#19632
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
Conversation
This actually caught a bug in the implementation of attributes on regular constants: A list of 4 constants with an attribute did not correctly result in an error, since the reallocated list wasn't stored anywhere.
Co-authored-by: David CARLIER <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RM approval
Technical review performed in my capacity as a normal developer, looks good to m
@@ -2956,7 +2956,7 @@ zend_ast * ZEND_FASTCALL zend_ast_with_attributes(zend_ast *ast, zend_ast *attr) | |||
/* Since constants are already stored in a list, just add the attributes | |||
* to that list instead of storing them elsewhere; | |||
* zend_compile_const_decl() checks the kind of the list elements. */ | |||
zend_ast_list_add(ast, attr); | |||
ast = zend_ast_list_add(ast, attr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! LGTM.
No description provided.