Cleanup/enforce formatting #531
Conversation
* change break on templates to multiline * aligning on consecutive macros * breaking before multiline strings adjust clang format Adjust clang format
…t, lib/support, system Preserve original formatting where original formatting clearly adds readability. These cases include: * block formatted comments, case statements, etc. * constant arrays and structures where wrapping lines mean something additional for readability
Preserve original formatting where original formatting clearly adds readability. These cases include: * block formatted comments, case statements, etc. * constant arrays and structures where wrapping lines mean something additional for readability * reformat the existing code to reinforce block formatting
…ice-control} Reinforce and preserve block-oriented formatting.
|
|
||
| /** | ||
| * Clear the structure, setting all members to 0. | ||
| */ |
There was a problem hiding this comment.
What is the point of enforcing prefix documentation for inline definitions? This is particularly onerous on readability.
I see a lot of places in this PR where the requirement is suppressed. If we don't like the way it looks, then why would we turn it on in the first place?
There was a problem hiding this comment.
This was one of the first files I changed, and I was still evolving a style. In a few places earlier on I thought the right path forward was to switch to prefix and rely on the clang format down the road. As the set of changes evolved, I've started using the // clang-format off directive more easily and I am happy to make those changes.
| @@ -30,6 +30,7 @@ | |||
|
|
|||
| #ifndef VERHOEFF10_NO_MULTIPLY_TABLE | |||
|
|
|||
| // clang-format off | |||
There was a problem hiding this comment.
What motivates turning off formatting here?
There was a problem hiding this comment.
Clang-format will attempt to reformat the sMultiplyTable below to fill the allotted columns (I believe the fill line is set at around 132 columns). This loses the readability of the table that is laid out to match Verhoeff encoding base.
| ::nl::Weave::Profiles::NetworkProvisioning::NetworkType | ||
| NetworkType; /**< The type of network. */ | ||
| int64_t NetworkId; /**< The network id assigned to the network by the device, -1 if not specified. */ | ||
| /** The type of network. */ |
There was a problem hiding this comment.
This was an example where the existing infix notation did not work well. I think the infix notation could have been fixed if we did not use fully qualified types, but as is the type varName; was just too long, especially with the type spilling into the infix doxygen. I'd be happy to refactor this back to infix with the shortened types, I think it is possible.
This is a bit of a bit request for comments / opinions. I've followed up on an earlier conversation with @turon on what OpenWeave code would look like if we started enforcing automated formatting. I've noted that there are a number of places in Weave where the textual layout of the file is particularly carefully though out and geared towards readability of the source file and associated inline documentation. This comes across particularly strongly in:
This pull request is a sweep through much of the openweave source code for places where the block oriented formatting significantly adds to the reliability. I did not attempt as yet the sweep through
toolsandtest-apps. With the changes in this PR, the clang-format produces the resulting source code layout as shown in https://github.com/robszewczyk/openweave-core/tree/cleanup/reformatted-source. If the automatically formatted source code results (broadly speaking) meet with the aesthetic and usability standards, I'd like to move in the direction of enforcing a uniform layout throughout the codebase.