Code style change: Replace clang-format off/on with TODO/TEST comments. by pgScorpio · Pull Request #2600 · jamulussoftware/jamulus

pgScorpio · 2022-04-17T14:05:41Z

Changes as discussed in issue #2591

Short description of changes
Replace clang-format off/on with TODO/TEST comments where applicable.

CHANGELOG: Internal: Code style: TODO and TEST comments instead of un-indenting with clang-format off/on

Context: Fixes an issue?
partly solves clang-format problems (#2587)

Does this change need documentation? What needs to be documented and how?
Maybe we should document this new coding-style rule somewhere?

Status of this Pull Request

Ready to merge
What is missing until this pull request can be merged?
Review

Checklist

I've verified that this Pull Request follows the general code principles
I tested my code and it does what I want
My code follows the style guide
I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
I've filled all the content above

ann0see

Yeah. Looks much better now.

Although it's a different idea to make the todo's stand out even more:

// // // TODO BEGIN \\ \\ \\
/* comment */
// // // TODO END  \\ \\ \\

might be even more visible. I think my proposal - which you implemented here is somewhat ok, but arguably "standard-ish" and not that outstanding (after looking at the diff. Surely, this would loose the editor highlighting)

linux/sound.cpp

ann0see · 2022-04-17T14:42:17Z

linux/sound.cpp

+                // TODO: BEGIN
+                /* do not call malloc in real-time callback
+                 */
+                // TODO: END


Suggested change

// TODO: BEGIN

/* do not call malloc in real-time callback

*/

// TODO: END

// TODO: BEGIN

/* do not call malloc in real-time callback */

// TODO: END

OK, so for single comments also the closing */ on the same line ?
But that is not according your proposal ;=))

Maybe have a second thought about that, since if we change this how consistent will it still be ?
If we place the comment closing at the end of line than multi line comment should be:

// TODO: BEGIN /* some comments and some more comment */ ... code... // TODO: END

to be really consistent.

I know. But after seeing it in real code, it seemed very strange.

ann0see · 2022-04-17T14:43:04Z

src/buffer.cpp

+    // TEST: BEGIN
+    /*
+    // TEST store important detection parameters in file for debugging
+    static FILE* pFile = fopen ( "test.dat", "w" );
+    static int icnt = 0;
+    if ( icnt == 50 )
+    {
+        fprintf ( pFile, "%d %e\n", iCurDecision, dCurIIRFilterResult );
+        fflush ( pFile );
+        icnt = 0;
+    }
+    else
+    {
+        icnt++;
+    }
+    */
+    // TEST: END


Yup. After some thinking this looks quite good.

ann0see · 2022-04-17T14:43:28Z

src/channel.cpp

+    // TODO: BEGIN
+    /* if we later do not fire vectors in the emits, we can remove this again
+     */


Suggested change

// TODO: BEGIN

/* if we later do not fire vectors in the emits, we can remove this again

*/

// TODO: BEGIN

/* if we later do not fire vectors in the emits, we can remove this again */

ann0see · 2022-04-17T14:43:40Z

src/channel.h

+    /* TODO needed for compatibility to old servers >= 3.4.6 and <= 3.5.12
+     */


ann0see · 2022-04-17T14:43:54Z

src/client.cpp

+    // TODO: BEGIN
+    /* needed for compatibility to old servers >= 3.4.6 and <= 3.5.12
+     */
+    Channel.CreateReqChannelLevelListMes();
+    // TODO: END


ann0see · 2022-04-17T14:44:30Z

src/main.cpp

+    // TEST: BEGIN
+    /* activate the following line to activate the test bench,
+     */
+    // CTestbench Testbench ( "127.0.0.1", DEFAULT_PORT_NUMBER );
+    // TEST: END


Previously this looked really nasty. Much better now.

ann0see · 2022-04-17T14:45:22Z

src/protocol.cpp

+    // TEST: BEGIN
+    /* channel implementation: randomly delete protocol messages (50 % loss)
+     */
+    // if ( rand() < ( RAND_MAX / 2 ) ) return false;
+    // TEST: END


Honestly, these tests should not be in the code but somewhere externally. But that's out of scope of this PR

ann0see · 2022-04-17T15:02:01Z

Maybe we should document this new coding-style rule somewhere?

Yes. CONTRIBUTING.md

pgScorpio · 2022-04-17T16:58:55Z

Although it's a different idea to make the todo's stand out even more:
// // // TODO BEGIN \\ \\ \\
/* comment */
// // // TODO END  \\ \\ \\
might be even more visible. I think my proposal - which you implemented here is somewhat ok, but arguably "standard-ish" and not that outstanding (after looking at the diff. Surely, this would loose the editor highlighting)

To be honest all those slashes and back-slashes make me dizzy, even nauseous if I look too long....
But of course there are MANY other possibilities, some even more 'outstanding':

//<<< TODO: BEGIN >>>
//>>> TODO: END <<<

/*** TODO: BEGIN ***/
/*** TODO: END ***/

/*### TODO: BEGIN ###*/   <- this one stands out really well I think
/*=== TODO: END ===*/

Surely, this would loose the editor highlighting

Not necessarily. For most highlighters the keywords are 'TODO:' and 'FIXME:' anywhere in any comment, so as long you keep it uppercase and directly followed by a colon, most highlighters will still work.

ann0see · 2022-04-17T17:01:15Z

I like /*** TODO: BEGIN ***/ since it looks standard-ish. However

/### TODO: BEGIN ###/

stands out well too. Would you prefer
/### TODO: BEGIN ###/ ?

pgScorpio · 2022-04-17T18:37:28Z

Would you prefer
/### TODO: BEGIN ###/ ?

Yes it stands out more, maybe we should combine them to make BEGIN stand out the most?

//### TODO: BEGIN ###//
//*** TODO: END ***//

ann0see · 2022-04-17T18:38:11Z

I'd prefer the consistent approach.

pgScorpio · 2022-04-17T18:47:49Z

I'd prefer the consistent approach.

OK it will be

//### TODO: BEGIN ###//
/* comment */
code...
//### TODO: END ###//

then ?
And what about */ end-of-comment?
Always at the end of the (last) comment line ?

ann0see · 2022-04-17T18:50:06Z

Yes. If the code already uses a specific multi line comment style this should be used. If not I'd go with your present proposal

pgScorpio · 2022-04-17T18:58:24Z

Yes. If the code already uses a specific multi line comment style this should be used.

Well most of them use the repeated asterisk:

/************
 * multi
 * line
 * comment
 ************/

But I think that's over-doing it here.

ann0see · 2022-04-17T19:05:14Z

If it's just "most" not all, I think we're fine with the lighter way — as long as clang-format is happy, I think it's ok.

pgScorpio · 2022-04-17T22:03:55Z

If it's just "most" not all, I think we're fine with the lighter way

Another option is not using block comments at all:

//### TODO: BEGIN ###//
// comment
// comment
code...
//### TODO: END ###//

I don't like using block comment inside the code anyway, since you can't use block comment to comment out a piece of code containing a block comment.

ann0see · 2022-04-18T06:15:41Z

That's also possible

ann0see

Great. Thanks. Looks good now.
Can the other PR with clang format be closed then?

ann0see · 2022-04-18T16:53:56Z

src/settings.cpp

-            // clang-format off
-// TODO compatibility to old version < 3.8.2
-if ( GetNumericIniSet ( IniXMLDocument, "server", "centservaddrtype", static_cast<int> ( AT_DEFAULT ), static_cast<int> ( AT_CUSTOM ), iValue ) )
-{
-    directoryType = static_cast<EDirectoryType> ( iValue );
-}
-else
-            // clang-format on
+
+            //### TODO: BEGIN ###//
+            // compatibility to old version < 3.8.2
            if ( GetNumericIniSet ( IniXMLDocument,
                                    "server",
-                                    "directorytype",
-                                    static_cast<int> ( AT_NONE ),
+                                    "centservaddrtype",
+                                    static_cast<int> ( AT_DEFAULT ),
                                    static_cast<int> ( AT_CUSTOM ),
                                    iValue ) )
        {
            directoryType = static_cast<EDirectoryType> ( iValue );
        }
+        //### TODO: END ###//
+
+        else if ( GetNumericIniSet ( IniXMLDocument,
+                                     "server",
+                                     "directorytype",
+                                     static_cast<int> ( AT_NONE ),
+                                     static_cast<int> ( AT_CUSTOM ),
+                                     iValue ) )
+        {
+            directoryType = static_cast<EDirectoryType> ( iValue );
+        }


The diff looks a bit strange due to reformatting here, but I think it's ok.

Yeah, I had too look 3 times too here...
Looks a bit strange because of the splitting up of if else.

If I remember well this was exactly the spot where also clang-format got confused ;=))

ann0see · 2022-04-18T16:56:55Z

Note to maintainers: This should be squash merged.

pgScorpio · 2022-04-18T17:12:13Z

Can the other PR with clang format be closed then?

Yes. I will close it..

ann0see · 2022-04-20T17:42:45Z

@hoffie @pljones could someone please review this PR?

hoffie

Thanks! I've got one minor nit/question -- see inline review comment.

I guess one could bike-shed about the proper style here. The syntax looks non-natural to me, but I guess one can get used to it. In the end, it doesn't matter that much and if done consistently -- which seems to be the case -- it can easily be changed if wanted.

Ideally, such changes would be ok'ed by all maintainers, but that's rather hard right now as per #2617.
As the issue and this PR have been open for some time now, I guess it's ok to merge even without more maintainer acknowledgements.

@pgScorpio I've got two general remarks:

If a commit and/or PR fixes an issue, you can use the "Fixes #0000" syntax in order to make Github link the PR to the Issue automatically. This will also auto-close the issue on PR-merge. I've done that manually here now.
Our CHANGELOG helper currently only understands single lines, i.e. the actual content has to stay on the same line as the keyword for it to be used. I'm therefore re-mentioning it here with slight variation.

CHANGELOG: Code style: Use TODO and TEST comments instead of un-indenting with clang-format off/on

Note to maintainers:

Should be squashed / commit message-edited
Might want to cherry-pick the add-to-authors commit from one of the other PRs here.

I can do both once the PR is fully approved.

windows/sound.cpp

ann0see · 2022-04-30T19:43:55Z

@hoffie @pgScorpio I think the last commit made clang-format fail???

pgScorpio · 2022-04-30T21:14:30Z

@annosee

@hoffie @pgScorpio I think the last commit made clang-format fail???

That's the other clang-format quirk again ! A 149 character long line on windows (CR/LF) is 150 chars long for clang-format !

Nothing I can do about that....
I think the only option is to use .gitatributes to force LF only on windows too.
See docs.github.com

EDIT: I re- clang-formatted on linux, so it should be ok now.

hoffie · 2022-05-15T20:21:27Z

Sorry for the long delay to re-review. Sadly, there are conflicts by now, most likely due to the sound backend moving. Therefore, I'm not clicking "approve" although this should be fine to do once the conflicts are fixed.

pgScorpio · 2022-05-15T20:39:56Z

@hoffie

Sorry for the long delay to re-review.
Sadly, there are conflicts by now, most likely due to the sound backend moving.

Yeah, the very disadvantage of waiting to long with merging PR's is the risk on conflicts ;=)
But I just rebased this PR, so it should be ok now.

hoffie

I haven't investigated why, but the diff of this PR looks really strange now.
On Github it appears to re-list changes from other PRs/commits and I can't tell why (e.g. .github/autobuild/android.sh).
On CLI, it looks less confusing but there are diff hunks which revert parts of another PR:

$ git diff upstream/master..pr2600 src/serverdlg.cpp
diff --git a/src/serverdlg.cpp b/src/serverdlg.cpp
index 968aa2f4..6aadcd28 100644
--- a/src/serverdlg.cpp
+++ b/src/serverdlg.cpp
@@ -175,16 +175,14 @@ CServerDlg::CServerDlg ( CServer* pNServP, CServerSettings* pNSetP, const bool b
     pbtRecordingDir->setWhatsThis ( "<b>" + tr ( "Main Recording Directory" ) + ":</b> " +
                                     tr ( "Click the button to open the dialog that allows the main recording directory to be selected.  "
                                          "The chosen value must exist and be writeable (allow creation of sub-directories "
-                                         "by the user %1 is running as)." )
-                                        .arg ( APP_NAME ) );
+                                         "by the user Jamulus is running as)." ) );
 
...

pgScorpio · 2022-05-15T21:46:41Z

I haven't investigated why, but the diff of this PR looks really strange now.

No idea what happened... I just did a rebase, and there was just one conflict in a comment. all other changes are by auto-merge...
And I guess it's normal that other changes due to rebase will show up here too?

ann0see · 2022-05-16T05:29:34Z

No. That's not normal. I've seen this quite often in the past and it always was a hint that this could lead to a huge screw up. What were the exact commands you were using?

pgScorpio · 2022-05-16T12:54:05Z

What were the exact commands you were using?

Just as usual:

git fetch upstream
git checkout <branch>
git rebase upstream/master
git status
git rebase --continue (when needed)
git add ...
[did I do a git commit here?, if so that might be the problem?]
git push --force

ann0see · 2022-05-16T19:17:22Z

[did I do a git commit here?, if so that might be the problem?]

Maybe? The PR in this state is not mergable unfortunately. Can you try the rebasing again or cherry pick the commits on a new branch?

pgScorpio · 2022-05-16T22:27:06Z

Can you try the rebasing again or cherry pick the commits on a new branch?

I did a reset of the last commit...
does that solve the problem ??

Fixes: jamulussoftware#2591

hoffie

Thanks @pgScorpio! Looks good now.
I've squashed the commits and cherry-picked your contributors list addition from another PR.

Will merge when CI is done/green.

ann0see self-requested a review April 17, 2022 14:38

ann0see reviewed Apr 17, 2022

View reviewed changes

ann0see added the refactoring Non-behavioural changes, Code cleanup label Apr 17, 2022

ann0see added this to the Release 3.9.0 milestone Apr 17, 2022

ann0see requested review from ann0see and hoffie April 18, 2022 14:43

ann0see approved these changes Apr 18, 2022

View reviewed changes

pgScorpio mentioned this pull request Apr 18, 2022

clang-format issues settings.cpp #2587

Closed

5 tasks

ann0see mentioned this pull request Apr 21, 2022

Code: Use clang-format 13 #2559

Closed

5 tasks

hoffie linked an issue Apr 26, 2022 that may be closed by this pull request

Coding style: Drop un-indented code in favor of // TODO: comments #2591

Closed

hoffie reviewed Apr 26, 2022

View reviewed changes

windows/sound.cpp Outdated Show resolved Hide resolved

ann0see requested a review from hoffie May 9, 2022 19:58

hoffie reviewed May 15, 2022

View reviewed changes

pgScorpio force-pushed the todo-comments branch from 2d7a4e0 to dc94cfc Compare May 16, 2022 22:19

pgScorpio and others added 2 commits May 21, 2022 00:17

Replace clang-format off/on with TODO/TEST comments

291bc55

Fixes: jamulussoftware#2591

util: Add pgScorpio to contributors list

d0d6f16

hoffie force-pushed the todo-comments branch from dc94cfc to d0d6f16 Compare May 20, 2022 22:27

hoffie approved these changes May 20, 2022

View reviewed changes

hoffie merged commit dbd34cd into jamulussoftware:master May 21, 2022

		/* TODO needed for compatibility to old servers >= 3.4.6 and <= 3.5.12
		*/

Comments

Conversation

pgScorpio commented Apr 17, 2022 • edited by hoffie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

ann0see left a comment • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgScorpio Apr 17, 2022 • 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!

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!

ann0see commented Apr 17, 2022

Uh oh!

pgScorpio commented Apr 17, 2022

Uh oh!

ann0see commented Apr 17, 2022

Uh oh!

pgScorpio commented Apr 17, 2022

Uh oh!

ann0see commented Apr 17, 2022

Uh oh!

pgScorpio commented Apr 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ann0see commented Apr 17, 2022

Uh oh!

pgScorpio commented Apr 17, 2022

Uh oh!

ann0see commented Apr 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pgScorpio commented Apr 17, 2022

Uh oh!

ann0see commented Apr 18, 2022

Uh oh!

ann0see left a comment

Choose a reason for hiding this comment

Uh oh!

ann0see Apr 18, 2022 • 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!

ann0see commented Apr 18, 2022

Uh oh!

pgScorpio commented Apr 18, 2022

Uh oh!

ann0see commented Apr 20, 2022

Uh oh!

hoffie left a comment • edited by ann0see Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ann0see commented Apr 30, 2022

Uh oh!

pgScorpio commented Apr 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pgScorpio commented Apr 17, 2022 •

edited by hoffie

Loading

ann0see left a comment •

edited

Loading

pgScorpio Apr 17, 2022 •

edited

Loading

pgScorpio commented Apr 17, 2022 •

edited

Loading

ann0see commented Apr 17, 2022 •

edited

Loading

ann0see Apr 18, 2022 •

edited

Loading

hoffie left a comment •

edited by ann0see

Loading

pgScorpio commented Apr 30, 2022 •

edited

Loading

pgScorpio commented May 15, 2022 •

edited

Loading

pgScorpio commented May 16, 2022 •

edited

Loading

hoffie left a comment •

edited

Loading