src: document headers members in src/util.h #33070
Conversation
nodejs-github-bot
added
the
c++
juanarbol
force-pushed
the
juanarbol/add-headers-information
branch
from
13881f5
to
dfe3f44
Compare
addaleax
added
author ready
There was a problem hiding this comment.
I'm not blocking, but in my experience, these comments are not useful. They are difficult to evaluate as to whether they are correct now, and they are impossible to keep up to date with changes. The std library headers declare many types, methods, functions, etc. Someone will use one of those types in a .cc file in node, it will be present, their code will compile, PR will merge. But, they will not go hunting through src/util.h (or any other headers) and update these comments, so the comments will become out of date very quickly.
Also, I don't see the comments having any value. Anyone who wants to know what features are declared by <cstring> can and will read the docs for that header, we don't need to document it.
The only time comments are needed on header includes is when they are being used for suprising reasons. Perhaps some system has a bug, and requires a header included where other systems would not, so a comment is needed so the include isn't removed during cleanup. Or perhaps, a symbol is availble in a non-standard location. Etc.
Sorry, but I've removed enough of these comments from old code bases where they have been found, and found completely out of date, and never found them to be useful. On the other hand, they don't actively harm a reader of the code, anyone reading can just ignore them as "probably out of date", so I don't object to adding them if some folks here think they can keep them up to date and find them useful.
|
to be honest, if we're deleting code like this, I'd definitely close this PR, It does not make sense, I'll be deleted soon. Or I could change this PR, I mean, @sam-github what do you think? |
|
IFor unused headers, deleting their include statements SGTM, totally uncontroversial. |
juanarbol
removed
the
author ready
juanarbol
force-pushed
the
juanarbol/add-headers-information
branch
from
dfe3f44
to
a667ed2
Compare
|
@sam-github you're welcome, thanks for your feedback, btw. |
juanarbol
added
the
author ready
PR-URL: #33070 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sam Roberts <[email protected]>
|
Landed 39ceb6a |
PR-URL: #33070 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sam Roberts <[email protected]>
PR-URL: #33070 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sam Roberts <[email protected]>
Document the used members of the headers in src/util.h,
also removes the assert header which is not being used.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes