Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:119343
Date: Thu, 19 Jan 2023 10:50:27 +0100
To: Rowan Tommins <rowan.collins@gmail.com>
Cc: internals@lists.php.net
Message-ID: <Y8kSY36lgLb0RWg2@swift.blarg.de>
References: <Y8U9B4AVP/R7vazI@swift.blarg.de>
 <CAGBsUrdQAXE2qk_UDQY9v6pYpWH+=bX6S3ou+sttQamtDC1F9A@mail.gmail.com>
 <Y8VI5yanzli8D4Cm@swift.blarg.de>
 <alpine.DEB.2.23.453.2301181536160.23089@singlemalt.home.derickrethans.nl>
 <Y8gZWLQd1SvL6a3n@swift.blarg.de>
 <8DA390C5-5E67-4847-A89F-1A8CCC6C5389@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <8DA390C5-5E67-4847-A89F-1A8CCC6C5389@gmail.com>
Subject: Re: [PHP-DEV] RFC: rules for #include directives
From: max+php@blarg.de (Max Kellermann)

On 2023/01/18 18:06, Rowan Tommins <rowan.collins@gmail.com> wrote:
> Then I guess you've never heard people quoting Robert "Uncle Bob" Martin, who is well known for asserting that code should be self-documenting, and therefore not need comments, saying things like:
> 
> > The proper use of comments is to compensate for our failure to express ourself in code. Note that I used the word failure. I meant it. Comments are always failures.
> 
> Many arguments could be had over whether that's going too far, but "too many comments" can definitely be a bad thing.

There is a tiny piece of truth in that, but it doesn't apply to
#include directives.  Yes, you should always try to write code in a
way that's simple enough for others to understand.  But #include
directives cannot be written in such a way.  Sometimes, you know why
an #include is there, but sometimes, you cannot, and only a comment
can explain it.

> To take the example Dmitry gave:
> 
> > #include "zend_portability.h" // for BEGIN_EXTERN_C
> 
> What should I understand from this comment?

That the reason for including this header was because we need
BEGIN_EXTERN_C in this file, and BEGIN_EXTERN_C is provided by that
header.  Without the comment, this would be hard to find out.

> - If I want some other symbol defined in zend_portability.h, do I need to adjust the includes? No, all the symbols are already imported. I could adjust the comment, but nothing will break if I don't.

I don't think that matters much.  If you use another symbol from
zend_portability.h, you could update the comment, or not.  Sure, if
you do not, the comment would be incomplete.  But that incomplete
comment is still better than no comment.

> - If I remove the last use of BEGIN_EXTERN_C in this file, can I remove the include? No, because other symbols from it might be used, even if they weren't when the comment was written.

Removing the last use of BEGIN_EXTERN_C would give you a hint that
maybe zend_portability.h does not need to be included anymore.  If you
care, you can decide to remove the #include and see if it still
builds.  If it doesn't there's apparently something else still needed
from zend_portability.h, and the error message tells you what is
needed.

If you still care, now's your chance to copy'n'paste that identifier
from the compiler's error message to the comment, replacing the old
comment.  Or just leave the old now-outdated comment.  And let
somebody else do it, eventually or never.

But still, an outdated comment doesn't hurt anybody.  It helps keeping
the code clean (by being able to easily see #include candidates which
can be removed), but if the comment turns out to be outdated, update
it or just leave it.

I mean, for the last 20+ years, nobody has cared for outdated and
unnecessary #includes.  Outdated and unnecessary #includes to have
negative effects.

And now some people pretend to care about outdated and unnecessary
comments, which have no negative effects.

> - Perhaps the name of the header doesn't give a clear enough idea of what symbols it might contain, and what types of file should include it. If so, concentrate on better naming, or better documentation of existing conventions.

Yes yes yes!

Concentrate on better naming - that's exactly what I'm about to do!
And that will lead to FEWER #include comments.

For example, many headers include zend_types.h but they only need
zend_result.  My PR contains a commit which moves this typedef to
zend_result.h.  Now I can change lots of "#include zend_types.h" to
"#inclued zend_result.h", and can then omit the comment, because it's
obvious why this header gets included.

(Dmitry has complained already about this very commit.)

> - Perhaps it is a justification added when the include was first added. If so, put it in the commit message and PR summary.

"Put it in the commit message" can be said about all code comments and
all code documentation.  I think that's wrong, because:

- commit message shows only the initial reason why the #include was
  added, but unlike code comments, commit messages cannot ever be
  updated

- looking up commit messages is more cumbersome than reading a code
  comment

A commit message should describe why something was done, and how
something was done; it is an important part of a software.  But it
should not replace documentation that helps understand the software.

Max