Add .mm extension to coding style check

[ngocdh]

Review of #1865 by @pljones revealed .mm extension was not checked.

[ngocdh]

Note that the coding style check for this PR won't pass because of the old iOS POC code, which is normal, but also proves that the proposed change works.

[ngocdh]

Team, I found the inplace edit option of clang-format (-i). Is it possible to use it to auto-correct style?

[ann0see]

Looks ok. @passing since you're the expert on clang format, could you please have a look at this too?

[passing]

Didn't notice there's mm files as well. If they can't have cpp as prefix, I guess it makes sense to include them in the style check as proposed. The PR should then just also update the style for these files.

Team, I found the inplace edit option of clang-format (-i). Is it possible to use it to auto-correct style?

We have considered automated fixing of the style (#1702), but due to different constraints agreed to not go for it at the moment.

[ann0see]

Ok. Thanks for the comment. No, .mm files can't have a .cpp extension as far as I know (they wouldn't be detected as objective c++ then --> no apple specific code available if we have a .cpp file).

On the automated formatting, I'd agree. But that can be a follow up PR.

[passing]

On the automated formatting, I'd agree. But that can be a follow up PR.

also fine, just better merge it directly after this PR then. Otherwise someone forking this repo in between might get a style violation for the non-compliant mm files!

[pljones]

It looks like in many places the expected style isn't being applied. I'm not sure why. If you look through the reformatting diff output, it's not right, I don't think. Maybe not only do the mm files need checking, the correct style needs selecting?

[ngocdh]

@pljones could you show an example where it’s not right?
The mm style check seems to work for me: https://github.com/jamulussoftware/jamulus/pull/1871/checks?check_run_id=2814420863

[pljones]

Possibly just me wanting even more space than the standard -- [...] always seems unspaced compared with everything else we do. So with all the [[NSProcessInfo processInfo] endActivity: pActivity->activityId]; stuff, I'm probably thinking there's something wrong when, according to the style guide, that's correct (it's styles as an subscript into an array). Perhaps there's something to be discussed there?

It results in lines like this:

+    pActivity->activityId = [[NSProcessInfo processInfo]
+        beginActivityWithOptions:options
+                          reason:@"Jamulus provides low latency audio processing and should not be inturrupted by system throttling."];

If that were a C-like cast, there would be much more spacing.

This bit I'd also have expected even more blank line deletion (i.e. inside the method):

--- ./ios/ios_app_delegate.mm	(original)
+++ ./ios/ios_app_delegate.mm	(reformatted)
@@ -7,9 +7,8 @@
 
 @implementation QIOSApplicationDelegate
 
-
-
-- (BOOL)application:(UIApplication *)application didFinishLaunchingWithOptions:(NSDictionary *)launchOptions {
+- (BOOL)application:(UIApplication*)application didFinishLaunchingWithOptions:(NSDictionary*)launchOptions
+{
 
     return YES;
 }

I'd also expect spacing inside the type casts (BOOL) -> ( BOOL ), etc.

[ngocdh]

I see, if there can be a better style for objective-c, then we should apply that.

This bit I'd also have expected even more blank line deletion (i.e. inside the method)

I suspect it's the same now for C++ code, not just .mm files.

[passing]

just found there's a few configuration options explicitly for Objective-C

• ObjCBinPackProtocolList
• ObjCBlockIndentWidth
• ObjCSpaceAfterProperty
• ObjCSpaceBeforeProtocolList

we could customize them in our style definition in case we are not happy with the defaults
(see https://releases.llvm.org/10.0.0/tools/clang/docs/ClangFormatStyleOptions.html)

[ngocdh]

It seems to be possible to detect ObjC (using RawStringFormats?) and define customized format for it (using Language: ObjC).

[pljones]

Example of what it's not touched that I don't like:

[[AVAudioSession sharedInstance] setMode:AVAudioSessionModeMeasurement error:&audioSessionError];

I'd rather this were

[ [AVAudioSession sharedInstance] setMode: AVAudioSessionModeMeasurement error:& audioSessionError ];

... but of course I've no idea if that syntactically viable ObjC...

[ann0see]

@emlynmac this is another one for you ;-).

Is the code @pljones mentioned valid Objective C?
#1871 (comment)

[emlynmac]

@emlynmac this is another one for you ;-).

This looks more like a personal preference around white-space. Valid code compiles, which I don't see an issue with here.

In terms of the code, the first version is much clearer to an Obj-C person than the second, i.e.

[[AVAudioSession sharedInstance] setMode: AVAudioSessionModeMeasurement error: &audioSessionError];

is preferred, or if you prefer smaller lines:

[[AVAudioSession sharedInstance] setMode: AVAudioSessionModeMeasurement
                                   error: &audioSessionError];

[ann0see]

Ok. Thanks for your input. The first option follows the Apple standards, while the second follows the strict Jamulus standards.
I'd not go with the Jamulus standards, but I'd leave the final decision up to @pljones and @ngocdh

[pljones]

Valid code compiles, which I don't see an issue with here.

This isn't about code validity, it's about style compliance. The whole of the Jamulus code base complies with a style guide which very, very few C++ developers would be comfortable with. But we live with it for the time being.

I think I'd rather say we'll make a separate style for Objective C that clang-format can check against, rather than try to have the same style applied to both, if it's not going to be adjusted to fit.

[ann0see]

What’s missing here? Can we merge this for the next release?

[softins]

What’s missing here? Can we merge this for the next release?

Although clang-format can be run on the .mm files, there is some discussion above about whether what it does to Obj-C is optimal. Maybe that needs to be resolved first.

[ann0see]

Concerning the comment by @pljones : I already stated that I'd prefer to stick to apple not Jamulus code style.

[ann0see]

Any updates here?

[pljones]

Unfortunately, the Github Actions with the runs on @ngocdh 's tree have expired -- they ran to clean completion but I'd have liked to see the output. I'm approving anyway.

[ann0see]

Ok. Rebased this.

[ann0see]

It works and I think it should be ok. Merging on this base.

[ann0see]

CHANGELOG: Internal: Check coding style on macOS/iOS code files (.mm extension)