Consolidate information from the three RFCs it is based on #11
Conversation
|
I've finally started doing some work on consolidating the content in these files - it would be helpful if you could have a look to make sure I didn't change the meaning. The original document used "should" as the RFC 2119 "MUST", which is rather unfortunate. I believe I have conveyed the original meaning though. Comments welcome! Once you're OK too, I'll put up a short RFC to ack/back this PR (and the ones for the other documents). |
coding-standards-and-naming.rst
Outdated
| or on GitHub. | ||
|
|
||
| Extensions may move between these three categories over time. ``hash`` and | ||
| ``json`` recently moved from "bundled" to "required" (though I believe |
There was a problem hiding this comment.
Recently as of when? The example should fix a release in which the move happened, for clarity.
coding-standards-and-naming.rst
Outdated
|
|
||
| Extensions may move between these three categories over time. ``hash`` and | ||
| ``json`` recently moved from "bundled" to "required" (though I believe | ||
| extensions never move out of the "required" category). ``sodium`` and ``ffi`` |
There was a problem hiding this comment.
The parenthetical also seems too personal for a policy document.
derickr
force-pushed
the
consolidate-coding-standards
branch
from
a104093 to
8b81759
Compare
|
@Crell, @iluuu1994, @Girgias — I have now also integrated CODING_STANDARDS.md too. I originally hadn't realised that the Policy Repository RFC referred to that as well. This text has been copied over mostly verbatim, but could potentially use some tweaks. |
coding-standards-and-naming.rst
Outdated
| (``int8_t``, ``int16_t``, ``int32_t``, ``int64_t`` and their unsigned | ||
| counterparts) are supposed to be available. | ||
|
|
||
| #. Functions that are given pointers to resources MUST NOT free them. |
There was a problem hiding this comment.
This is kind of a silly rule. We often transfer ownership of a value to some other function. I agree that this should be documented, the programmer currently usually needs to read the implementation to understand whether ownership is transferred. We may potentially improve this with annotations. Clang has some annotations, but they are not particularly well documented. https://releases.llvm.org/15.0.0/tools/clang/docs/AttributeReference.html#id686
There was a problem hiding this comment.
I agree that we should change this, but this PR isn't about changing existing coding standards.
coding-standards-and-naming.rst
Outdated
| with ``strlen()``. Write your functions in such a way so that they'll take | ||
| advantage of the length property, both for efficiency and in order for them | ||
| to be binary-safe. Functions that change strings and obtain their new lengths | ||
| while doing so, should return that new length, so it doesn't have to be |
There was a problem hiding this comment.
This is very outdated, since we have zend_string.
There was a problem hiding this comment.
Yeah, it is. CODING_STANDARDS hadn't been updated for a while, I suppose.
coding-standards-and-naming.rst
Outdated
| while doing so, should return that new length, so it doesn't have to be | ||
| recalculated with ``strlen()`` (e.g. ``php_addslashes()``). | ||
|
|
||
| #. You MUST NOT use ``strncat()``. If you're absolutely sure you know what |
There was a problem hiding this comment.
Weird call-out, there are so many other dangerous C functions. 🙂
There was a problem hiding this comment.
It was in the original document. It probably has a 20 year old history as to why it is here!
coding-standards-and-naming.rst
Outdated
| 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: |
There was a problem hiding this comment.
preferably two
We don't do this anywhere. Can we drop that?
There was a problem hiding this comment.
I'm okay dropping that, but this PR isn't about changing existing rules. Perhaps it makes sense to put these changes in the RFC that I will send to internals?
coding-standards-and-naming.rst
Outdated
| MUST follow the standard prefixing conventions during their initial | ||
| implementation. | ||
|
|
||
| The file labelled ``EXPERIMENTAL`` SHOULD include the following information: |
There was a problem hiding this comment.
- It's unclear to me whether this refers to internal or user functions.
- We have never done this in practice, and I think we should completely drop this section.
There was a problem hiding this comment.
We had a lot of extensions marked as EXPERIMENTAL in the olden days, but that should no longer happen. Only stable extensions should be added to php-src. New functions go into master only, and master is somewhat "experimental" anyway, so I agree that there is no need for this section.
There was a problem hiding this comment.
I agree. I think we should rewrite this section, and rewrite it to:
New extensions MUST start out as third-party extensions, or in an experimental branch, until an RFC is passed to add them to the core distribution.
There was a problem hiding this comment.
I also saw a few of these legacy comments. But I didn't want to make changes in this PR just yet.
I suppose the best way is to list all the changes on top of the consolidation in the RFC that I will send to internals. And only then make these as suggested in separate commits?
coding-standards-and-naming.rst
Outdated
| (``int8_t``, ``int16_t``, ``int32_t``, ``int64_t`` and their unsigned | ||
| counterparts) are supposed to be available. | ||
|
|
||
| #. Functions that are given pointers to resources MUST NOT free them. |
There was a problem hiding this comment.
I agree that we should change this, but this PR isn't about changing existing coding standards.
coding-standards-and-naming.rst
Outdated
| with ``strlen()``. Write your functions in such a way so that they'll take | ||
| advantage of the length property, both for efficiency and in order for them | ||
| to be binary-safe. Functions that change strings and obtain their new lengths | ||
| while doing so, should return that new length, so it doesn't have to be |
There was a problem hiding this comment.
Yeah, it is. CODING_STANDARDS hadn't been updated for a while, I suppose.
coding-standards-and-naming.rst
Outdated
| while doing so, should return that new length, so it doesn't have to be | ||
| recalculated with ``strlen()`` (e.g. ``php_addslashes()``). | ||
|
|
||
| #. You MUST NOT use ``strncat()``. If you're absolutely sure you know what |
There was a problem hiding this comment.
It was in the original document. It probably has a 20 year old history as to why it is here!
coding-standards-and-naming.rst
Outdated
| 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: |
There was a problem hiding this comment.
I'm okay dropping that, but this PR isn't about changing existing rules. Perhaps it makes sense to put these changes in the RFC that I will send to internals?
coding-standards-and-naming.rst
Outdated
| MUST follow the standard prefixing conventions during their initial | ||
| implementation. | ||
|
|
||
| The file labelled ``EXPERIMENTAL`` SHOULD include the following information: |
There was a problem hiding this comment.
I agree. I think we should rewrite this section, and rewrite it to:
New extensions MUST start out as third-party extensions, or in an experimental branch, until an RFC is passed to add them to the core distribution.
|
I'm not too stoked discussing changes of this document with internals, because it's really not relevant to the vast majority of people... |
|
Maybe we need a real-internals mailing list. ;) |
|
I was just going to say that these changes are made to follow current practise. And not make it a discussion point. I doubt anybody would bring them up, and disagree with what we have been doing for ages.
|
derickr
force-pushed
the
consolidate-coding-standards
branch
from
ac8c52f to
f534be7
Compare
| New extensions MUST start out as third-party extensions, or in an experimental | ||
| branch, until an RFC is passed to add them to the core distribution. |
There was a problem hiding this comment.
I would scrap the whole section as it's just given that you cannot introduce a new extension without the RFC so the rest are just implications that don't need to be specified here.
| #. Functions that are given pointers to resources SHOULD NOT free them, unless | ||
| this is documented. |
There was a problem hiding this comment.
This really depends on the function and logic. I think this whole think should be removed as well as it's context dependent.
| #. You SHOULD use the ``smart_str_*`` family of functions for string creation, | ||
| instead of relying on the C-library versions, such as ``strncat()``. |
There was a problem hiding this comment.
smart_str_* should be used only if the string is constructed with multiple variable pieces. Otherwise basic creation should be done using zend_string_alloc and similar (e.g. short concatenations using zend_string_concat2 and zend_string_concat3).
That said I'm not really sure why this is in coding standard. This should really be part of some extension documentation like in doc/ or php internals book.
No description provided.