add clang-format style definition #1127
Conversation
|
I looked at some of the code that was changed by this lint'er and was not impressed.
|
|
@passing Thanks for your PR! Will look at it, but as it is rather large (as expected), it may take some time. :) @DavidSavinkoff
Did you have a look at the referenced issue? It provides some more context. The goal of this PR is to work out the details, e.g. where things have to be excluded from re-formatting
I guess one can have different opinions on that. I'm very thankful for @passing to work on that. |
|
Windows build failed for some reason: |
Is this caused by the header re-ordering, I wonder? |
|
Sorry for leaving this a bit behind. I think we should get this in, but this should be planned carefully as it has the chance of annoying all other contributors submitting PRs. I would suggest to merge this PR (which should be easy to re-base as can be done automatically, right?) when it is ready and shortly before tagging the next beta (i.e. when all other larger PRs have settled). Could you rebase again now so that we can see if the Windows build works now after adjusting the include order? |
|
I think this should be tested for at least 2 weeks by someone who can build Windows stuff, and who can knowledgably inspect every Windows header file on a separate fork. This is not a small change. It should be slated for Jamulus 3.7.2 unless you get it done sooner and still have time. That way you don't get overly optimistic. |
There is no guarantee that the Clang #include order parser will give the same results twice with the same input:
|
We don't have static planning for releases. I'm trying to move this forward with the target of getting it into the next version. If it isn't ready in time, we will just move it to the next version. If we want this to get it in at all (and I think there is consensus that we want to), we have to start at some point. I'm asking for a rebase now in order to find out if the recent adjustments already improved things for the Windows build. It's very likely that we will need another rebase afterwards.
I do not consider such a statement to be nice and I ask you to stick to technical arguments (which are valuable!). |
|
Sorry for being mean, I got too concerned. I'll let it be. |
|
Windows build still fails. :( Should we try without include re-ordering and see if it helps? This would already be a huge improvement. We could review the header re-ordering in a future PR and try to find the culprit. |
I had tried it with disabling include re-ordering in |
|
Seems like it fixed it! :) |
|
Need to get the conflicts resolved (and preserve the green build). |
|
conflicts resolved - did that by rewinding my branch locally, fetching/merging from upstream and running clang-format on that: |
FYI: That can be done as Over all, you could have "started again" like this (again, just for "a different way to do it"): |
There was a problem hiding this comment.
Believe it or not, I have finally managed to go through the whole diff and scan all changes. I have left lots of possibly critical-sounding comments, but I want to reiterate that this PR is a huge improvement. It fixes tons of formatting errors which have slipped through over time, it makes things consistent and improves readability in lots of places. There are lots of places where things are simply formatted differently, without much up- or downsides.
I'm not expecting anyone else to read the whole diff, but maybe someone can check the places where I've added comments (especially @pljones and @softins)?
I think the following should still be done:
- Increase permitted line length. Lots of code is broken into multiple lines just because the line length exceeds the limit (I assume). This even affects simple assignments with long variable names. It also affects lines which have inline-comments at the end. I think we should be more liberal here. If there is a way to exclude such comments from the counting, this could be a good idea. Otherwise I would suggest setting a really large limit (180?) to allow for those cases. I don't think overly long lines are a problem in PRs, currently. Maybe @passing can check some of the cases I marked and find a good value?
- I've found some edge cases which simply get worse due to the formatter. I suggest that the formatter should be disabled there (I've commented accordingly). @passing Can you do that?
- Some previously aligned variable, constant and initializer assignments get un-aligned and I don't know why. Maybe @passing has another idea what to do about them (commented on them as well)?
- Indentations in preprocessor macros are dropped. Check if they can be kept somehow.
CI was green last time I checked and I would not expect otherwise as all changes clearly seem to be formatting/wrapping changes (no include re-ordering anymore).
Other than that, this is really close to being merge-able, I think.
android/sound.cpp
Outdated
| QString sDirection = | ||
| ( stream->getDirection() == oboe::Direction::Input ? "Input" | ||
| : "Output" ); | ||
| QString sFramesPerBurst = QString::number ( stream->getFramesPerBurst() ); | ||
| QString sBufferSizeInFrames = | ||
| QString::number ( stream->getBufferSizeInFrames() ); | ||
| QString sBytesPerFrame = QString::number ( stream->getBytesPerFrame() ); | ||
| QString sBytesPerSample = QString::number ( stream->getBytesPerSample() ); | ||
| QString sBufferCapacityInFrames = | ||
| QString::number ( stream->getBufferCapacityInFrames() ); | ||
| QString sPerformanceMode = | ||
| ( stream->getPerformanceMode() == oboe::PerformanceMode::LowLatency | ||
| ? "LowLatency" | ||
| : "NotLowLatency" ); | ||
| QString sSharingMode = | ||
| ( stream->getSharingMode() == oboe::SharingMode::Exclusive ? "Exclusive" | ||
| : "Shared" ); | ||
| QString sDeviceID = QString::number ( stream->getDeviceId() ); | ||
| QString sSampleRate = QString::number ( stream->getSampleRate() ); | ||
| QString sAudioFormat = | ||
| ( stream->getFormat() == oboe::AudioFormat::I16 ? "I16" : "Float" ); | ||
| QString sFramesPerCallback = | ||
| QString::number ( stream->getFramesPerCallback() ); |
There was a problem hiding this comment.
Sadly, this is is still an example where readability is worse then before. I assume that this is still due to the line length limit?
There was a problem hiding this comment.
I think widening will solve a lot. But the wrapping rules still need to be "right". Line 142 is actually wrongly written - either the (/) pair aren't needed or should surround the boolean expression, not the whole expression - but we're not getting that kind of correction "for free" ;). Line 144 is probably correctly wrapped - if it were long enough.
I don't know what the line width for wrapping is set to, but I'd aim for 120 minimum and then we can try increasing in steps until we're happy. (Yes, I saw you said something about that, @hoffie ;) )
There was a problem hiding this comment.
Indentations in preprocessor macros are dropped. Check if they can be kept somehow
having them exactly like they were is not possible, but could be changed to one of these two options:
https://github.com/passing/jamulus/blob/b50bafc55cbd1ec253a51b4065b07aa16d2babf0/test_pp_before_hash/src/server.cpp#L35-L42
https://github.com/passing/jamulus/blob/b50bafc55cbd1ec253a51b4065b07aa16d2babf0/test_pp_after_hash/src/server.cpp#L35-L42
- TODO: decide on preprocessor directive indenting
There was a problem hiding this comment.
I don't know what the line width for wrapping is set to, but I'd aim for 120 minimum ...
totally agree it should be 120 (or more)
The code from above would then look like this:
https://github.com/passing/jamulus/blob/b50bafc55cbd1ec253a51b4065b07aa16d2babf0/test_120/android/sound.cpp#L118-L139
going to 180 would result in
https://github.com/passing/jamulus/blob/b50bafc55cbd1ec253a51b4065b07aa16d2babf0/test_180/android/sound.cpp#L114-L132
so in this situation it would even align all the assignments - but it just gets pretty wide then
There was a problem hiding this comment.
Thanks! At least in this case the latter (180) looks more readable to me. Let's see what @pljones thinks.
android/sound.cpp
Outdated
| QString latencyMode = ( stream->getPerformanceMode() == oboe::PerformanceMode::None ? "None" : "Power Saving" ); | ||
| QString latencyMode = | ||
| ( stream->getPerformanceMode() == oboe::PerformanceMode::None | ||
| ? "None" | ||
| : "Power Saving" ); |
There was a problem hiding this comment.
I quite like it, actually :).
But I agree about making the code wider. If it's happening, it's happening with this change.
There was a problem hiding this comment.
one line with ColumnLimit 120
android/sound.h
Outdated
| static const uint8_t RING_FACTOR; | ||
| CSound ( void (*fpNewProcessCallback) ( CVector<short>& psData, void* arg ), | ||
| CSound ( void ( *fpNewProcessCallback ) ( CVector<short>& psData, |
There was a problem hiding this comment.
Well, the syntax appears to have beaten the formatter here! The identifier, fpNewProcessCallback, should be aligned with arg and strMIDISetup below. (This declaration's psData and arg are, I think, redundant?)
There was a problem hiding this comment.
looks different now with ColumnLimit: 120 - but not sure if better
linux/sound.cpp
Outdated
| ( input_port_right == nullptr ) || | ||
| ( output_port_left == nullptr ) || | ||
| ( output_port_right == nullptr ) ) | ||
| input_port_left = jack_port_register ( pJackClient, |
There was a problem hiding this comment.
The original was accepted layout. Is there any way to teach that to clang? Or did a max width trigger it?
There was a problem hiding this comment.
I don't think there's a way to align by ||
| @@ -182,65 +196,68 @@ class CAudioMixerBoardSlots : public CAudioMixerBoardSlots<slotId - 1> | |||
| }; | |||
|
|
|||
| template<> | |||
| class CAudioMixerBoardSlots<0> {}; | |||
| class CAudioMixerBoardSlots<0> | |||
There was a problem hiding this comment.
Single line preferred in this situation, if that's possible. (i.e. empty body, not too wide)
There was a problem hiding this comment.
fixed that by adding
AllowShortBlocksOnASingleLine: Empty
There was a problem hiding this comment.
Hm - is this another v12 feature?
src/audiomixerboard.h
Outdated
| public: | ||
| CMixerBoardScrollArea ( QWidget* parent = nullptr ) : QScrollArea ( parent ) {} | ||
| public: | ||
| CMixerBoardScrollArea ( QWidget* parent = nullptr ) : |
There was a problem hiding this comment.
Again, if not too wide, single line preferred. However, that parent class reference may take it over. Let's say it does. In that case, the {} would be on the same line as the parent reference. I think...
There was a problem hiding this comment.
also fixed with AllowShortBlocksOnASingleLine: Empty
There was a problem hiding this comment.
In summary, I'm really happy with these changes now:
- Lots of places become more readable.
- Lots of places become more correct according to the existing rules.
- Lots of places get changed where it is hard to argue if the previous or the new formatting is better.
- The places where existing, special formatting must be kept, are marked accordingly and are not re-formatted as expected.
- Some things may be a bit worse than carefully hand-crafted formatting. However, this is hard to keep and may still be improved by future clang versions or formatter options. It does not make sense to stop those places from automatic formatting as we don't lose any information there.
Thanks for working on this! Approving now, but also noting that this should be the very last PR to be rebased, re-formatted and merged before rc1.
@passing Would you be available sometime on Sunday evening (CEST) by chance to do the last rebase/re-format?
| @@ -1135,41 +1591,41 @@ QString CLocale::GetCountryFlagIconsResourceReference ( const QLocale::Country e | |||
| strReturn = ""; | |||
| } | |||
|
|
|||
| // AT LEAST QT 4.8 IS REQUIRED: | |||
There was a problem hiding this comment.
Given we've moved to 5.9 minimum, I think we could delete this entire block that's commented out. 1138 to 1172 old / 1594 to 1628 new.
There was a problem hiding this comment.
not sure if we should start removing outdated comments in this PR...
|
There are some bits of "self-documenting code" that the formatting breaks that I think are worth preserving. There are some flagged "tests", which should be treated like the "TODO" items. And there are some "to do" items that aren't flagged as "TODO" items but should be. Plus one block of code that's been commented out doing nothing for far too long and, as we're making large scale changes, it's better to delete than simply indent... Oh, and if the MT code gets merged first, server.cpp will be an interesting rebase. |
to achieve better alignment and prevent clang-format from merging lines
|
@passing I see that you've already updated/rebased the PR an hour ago. We do not plan to merge any other code changes for 3.8.0, so this should have been the final rebase. Thanks! :) As all feedback seems to be addressed, @pljones can just merge once he had a look. |
|
I'll check back in a while. I'm going to approve when I do check, though :). |
| # include <QDebug> | ||
| # include <stdio.h> | ||
| # include <math.h> | ||
| # include <string> |
There was a problem hiding this comment.
Is this expected? It looks strange to me having # separated from include.
There was a problem hiding this comment.
Is this expected? It looks strange to me having
#separated frominclude.
Yes, I think it was the decision to do that when discussing the options (no indentation at all, indent with #, indent but keep # on first column). I wanted to link the discussion here, but I suspect it's hidden in one of the conversations as part of this PR.
Such things could be changed again in a rather simple way later, if we decide otherwise. I think we should really focus on getting this long-term, huge PR merged. It will not be the last one. :)
There was a problem hiding this comment.
Found one part of the conversation here, for reference: #1127 (comment)
lint all code with defined style
prerequisite for #901