On Tue, 15 Nov 2022, at 11:41, Daniel Kinzler wrote:
Am 10.11.2022 um 03:08 schrieb Tim Starling:
Clutter, because it's redundant to add a
return type declaration when the return type is already in the doc comment. If we stop
requiring doc comments as you propose, then fine, add a return type declaration to methods
with no doc comment. But if there is a doc comment, an additional return type declaration
just pads out the file for no reason.
I agree that we shouldn't have redundant
doc tags and return type declarations. I would suggest that all methods should have a
return type declaration, but should not have a @return doc tag unless there is additional
info […]
The performance impact is measurable for hot
functions. In gerrit 820244
<https://gerrit.wikimedia.org/r/c/mediawiki/core/+/820244> I removed parameter type
declarations from a private method for a benchmark improvement of 2%.
This raises an interesting issue, one that has bitten me before: How do we know
that a given method is "hot"? Maybe we should establish a @hot or @performance
tag to indicate that a given method should be optimized for speed. […]
I think the enforced and automated codesniffer could remain fairly simple: As today, the
sniff encourages all methods to have parameter and return types documented in a way that
humans, Phan, and IDEs can understand for static analysis to avoid and catch mistakes.
What I propose we change is that instead of enforcing this solely through a mandatory doc
comment, enforce it by requiring at least one of them to be present. Either parameters and
returns are typed, or a doc block exists. Both may exist, of course.
We've established in this email thread that it can be cluttering (and waste of effort)
to require repeating of information when doing so adds no value. It is also my
understanding that Phan and IDEs already understand either and both so we don't need
them to be aware of which "should" exist.
Is there value in enforcing removal of existing doc blocks after someone has written it?
This seems to me like potentially a significant time sink with no return on that other
because we enforced it as a new rule. If we agree there is no urgency in removing existing
doc blocks or actively blocking CI when someone choose to write a doc block, then afaik we
do not need new annotations like "hot" or "performance" or some other
tag to surpress warnings about doc blocks.
I do think it is important to preserve author intent when it comes to performance
optimisations. However these are by no means limited to this new notion of saving native
type overhead. There are all sorts of code optimisations. I believe we typically document
these through an inline comment like "Optimization: ..." next to the code in
question, in which the need for optimisation and sometimes (if non-obvious) how that
optimisation is achieved, are mentioend. That should suffice I think in preserving the use
case and e.g. prevent someone from re-introducing typing where it was previously removed
for perf reasons.
In other words: Codesniffer helps us avoid unknown types (in docblock and/or native type),
and inline comments remind us about past performance optimisations. Do we need more? If
so, what is the benefit/usecase for more? What do we risk if we don't?
-- Timo