Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kernel: convert many C comments to C++ comments #5271

Merged
merged 2 commits into from
Dec 18, 2022

Conversation

fingolfin
Copy link
Member

Why? For the inline comments deal with in the first commit (made using a perl invocation) the reasoning is this: Adjusting comments like

    /* catch infix operations                                          */

or

    UInt                t1;             /* type of left  operand           */
    UInt                t2;             /* type of right operand           */
    ArithMethod2        func;           /* mod function                    */

to always fill exactly the correct number of columns is a major PITA. Failing to shorten such a comment also interacts badly with clang-format..

For multi-line comments, I usually also find it easier to come up with a uniform styling when using C++ comments; for multiline C comments, we have a plethora of ways to format them. Which of course isn't a serious problem on its own, except it adds mental load ("how should I format this comment??")

Note that this PR deliberately does not touch the big "block" comments of this kind (other than "fixing" some with "obvious" deviations):

/****************************************************************************
**
*F  SyRmdir( <name> )  . . . . . . . . . . . . . . .  remove directory <name>
*/

I actually also have a script converting those, but this may require more discussion and so I wanted to keep it seperate.

@fingolfin fingolfin added topic: kernel release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Dec 14, 2022
src/costab.c Outdated Show resolved Hide resolved
src/costab.c Outdated
Int rc; // right coset to apply to
Int tc; // temporary coset
Int i; // loop variable
Obj hdTmp; // temporary variable

/*T 1996/12/03 fceller this should be replaced by 'PlistConv' */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one not handled?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, indeed. This is because of the T. We have to decide how to treat such comments. Presumably could be converted to // TODO: . Does that seems sensible?

Just to explain, the main part of this (in the first commit) was created using a very simple perl invocation. The idea is to take care of the 95% of "easy" cases in a fully automated way, to make it easy to rebase the PR if needed. Then the remaining cases may need some more manual work. Of course I can add more "automated" changes, but I'd like to keep each separate transformation as simple as possible.

@ChrisJefferson
Copy link
Contributor

i'm happy for this to be merged as-is, and my comments to be dealt with later, just wanted to raise them as I checked to make sure nothing looked broken.

@fingolfin
Copy link
Member Author

Ideally I'd like to get PR #5256 merged first, as it is easier to deal with resulting conflicts in this PR here (as most of the changes can be easily re-created by re-running a script.

This commit was generated using this command:

    perl -pi -e 's;/\*[ \t]+(.+[^ \t])[ \t]+\*/[ \t]*$;// \1;g' src/*.* src/hpc/*.*
- convert more C comments to C++ comments
- remove empty comments
- remove some weird "patterns" at the end of some C++ comments
- fix some indentation issues and other minor stuff in a bunch of comments
- remove some obsolete comments
@fingolfin fingolfin enabled auto-merge (rebase) December 16, 2022 16:29
@fingolfin
Copy link
Member Author

This is now good to go from my POV

@fingolfin fingolfin disabled auto-merge December 18, 2022 22:41
@fingolfin fingolfin enabled auto-merge (rebase) December 18, 2022 22:41
@fingolfin fingolfin disabled auto-merge December 18, 2022 22:42
@fingolfin fingolfin merged commit 035f52e into gap-system:master Dec 18, 2022
@fingolfin fingolfin deleted the mh/reformat branch December 18, 2022 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants