Remove protos and folding marks from .c files #5532

kocsismate · 2020-05-06T09:15:57Z

No description provided.

cmb69 · 2020-05-06T09:20:47Z

TSRM/TSRM.c


-/* {{{ */
+/* */


This doesn't look right; the comment should be removed altogether.

Oops, thanks! I'll fix it up manually :) Probably there will be some other small glitches like this.

cmb69 · 2020-05-06T09:21:33Z

Zend/zend_closures.c


-/* {{{ proto mixed Closure::call(object to [, mixed parameter] [, mixed ...] )
-   Call closure, binding to a given object with its class as the scope */
+


It might be better to remove the empty line.

I tried my best to remove empty lines that were added but I didn't always manage to do so :( I'll have another look though

I've just seen that there are way too many new lines added :/ So probably I'll have to retry as soon as we agree that protos/folding marks are ok to remove

carusogabriel · 2020-05-06T09:30:39Z

ext/calendar/easter.c

-/* {{{ proto int easter_date([int year])
-   Return the timestamp of midnight on Easter of a given year (defaults to current year) */
+


Does it make sense to drop functions' descriptions?

Yes, I also wasn't exactly sure. But it seems that neither these descriptions are that useful

IMHO, these "protos" should be removed, because they are just comments, and as such not rarely out-dated. Also, I believe these have been introduced before we had ZPP, and as such were useful in their own right. Furthermore, they are no longer used to generate the docs, to my knowledge. However, all that might better be discussed on the mailing list.

Right, I feel like proto comments are more wrong than correct nowadays :) We update the stubs, but we don't update the proto comments...

It might make sense to keep the description comment though.

I'll have to redo the changes because many unintended new lines sneaked in despite my efforts, so I'll try to be considerate to those comments next time

derickr

This seems like a really pointless thing to do. Sure, by all means don't add new ones, and clean ones out when you're updating a bit of the code, but this just adds a mess of conflicts further down the line. I do not believe this should ever be merged.

nikic · 2020-05-06T10:28:45Z

@derickr Based on conversation in chat, you had two concerns about this change:

Merge conflicts. This is always a potential complication with mass changes. I think it will not be particularly problematic in practice, because to the most part this only affects function signatures, which is something we only rarely change in stable branches (a large part of them cannot be changed due to ABI stability guarantees). The PHP 7.4 -> master merge in particular is already somewhat prone to conflicts, due to migration to stubs, due to mass changes in error messages and due to mass test reindendation. I don't think this particular change will rise above the noise floor caused by those other changes. Finally, in practice, nearly all upwards branch merges are performed by either @cmb69 or myself, and we're (I believe, correct me if I'm wrong Christoph) prepared to deal with potential merge conflicts.
Tracing history. I think this change will have relatively little impact on that. Viewing the blame for zend_generators.c after this change, we can see some lines attributed to this change. However, a click on the "View blame prior to this change" button removes them. Especially as function signatures only make up a small part of the file, this also doesn't seem like a big problem in practice to me.

I'm not a big fan of only removing the folder marks when touching a piece of code, because it means that we mix style changes with functional changes (something we generally discourage), and because it means that the codebase will spend a long time in an intermediate state where some functions have folder marks and some don't, often in the same file. I don't think that is a good state to be in.

laruence · 2020-05-08T08:06:48Z

for someone like me, who use VIM as primary editor, folder marks are really useful.
-1 for this.

nikic · 2020-06-16T14:27:01Z

Looks like we don't have consensus here :(

@laruence I also use vim as primary editor...

Vim supports foldmethod=syntax, which unfortunately has issues on some files in php-src due to bad use of preprocessor conditions. Those are generally easy to fix, and I think long term, it will be much less effort to fix the few places where this happens rather than maintaining thousands of folding marks across the whole codebase.

kocsismate · 2020-06-16T14:30:29Z

@nikic What I wanted to propose as soon as my other stuff is done is to only remove protos - both from c files and tests - and leave alone folding marks. Would it be a good middle ground for both sides? :)

nikic · 2020-06-16T14:33:05Z

@kocsismate Sounds reasonable to me.

kocsismate · 2020-06-24T06:30:43Z

I'm closing this since the changes were somewhat problematic.

cmb69 reviewed May 6, 2020

View reviewed changes

kocsismate force-pushed the remove-proto branch from 6a2acd6 to 78a73e7 Compare May 6, 2020 09:29

carusogabriel reviewed May 6, 2020

View reviewed changes

derickr requested changes May 6, 2020

View reviewed changes

kocsismate force-pushed the remove-proto branch from 78a73e7 to b95cd95 Compare May 6, 2020 09:34

kocsismate force-pushed the remove-proto branch 2 times, most recently from 2fcc3b3 to 25528e4 Compare May 7, 2020 06:59

kocsismate added 2 commits May 7, 2020 09:38

Remove protos

bec9d88

Remove folding marks

5c51bf7

kocsismate force-pushed the remove-proto branch from 25528e4 to 5c51bf7 Compare May 7, 2020 07:39

kocsismate mentioned this pull request Jun 23, 2020

Remove proto comments #5758

Closed

kocsismate closed this Jun 24, 2020


		/* {{{ proto mixed Closure::call(object to [, mixed parameter] [, mixed ...] )
		Call closure, binding to a given object with its class as the scope */

		/* {{{ proto int easter_date([int year])
		Return the timestamp of midnight on Easter of a given year (defaults to current year) */

Remove protos and folding marks from .c files #5532

Remove protos and folding marks from .c files #5532

Uh oh!

Conversation

kocsismate commented May 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kocsismate May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kocsismate May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derickr left a comment

Choose a reason for hiding this comment

Uh oh!

nikic commented May 6, 2020

Uh oh!

laruence commented May 8, 2020

Uh oh!

nikic commented Jun 16, 2020

Uh oh!

kocsismate commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic commented Jun 16, 2020

Uh oh!

kocsismate commented Jun 24, 2020

Uh oh!

Uh oh!

kocsismate May 6, 2020 •

edited

Loading

kocsismate May 6, 2020 •

edited

Loading

kocsismate commented Jun 16, 2020 •

edited

Loading