Proposed clang-format file

[asmaloney]

Describe the feature you would like

As we've long-discussed, some form of auto-formatting is required.

I've created the following clang-format file as a starting point based off of our current style and the things we've discussed updating in the past. (Here's the option reference.)

How you use it on your platform/IDE is going to be different, but this file lives at the top-level of the repo either as .clang-format or _clang-format. I've chosen the latter for now so that it's easier to work with.

Please keep in mind that we're not all going to agree on the format 100% - and we're at the mercy of clang-format's limitations in some ways.

The goal is to come up with one we can live with to save all the spacing/formatting pain!

Example branch: clang-format

[asmaloney]

Does anyone have input on this?

Would it help if I applied it to a branch so you can just switch branches and check it out?

[WargodHernandez]

I'd appreciate a branch to test it out :+1:

[asmaloney]

Ok - there's a branch to take a look at.

I see one odd indentation issue with old C-style comments /** when \param spans several lines, but I can live with that.

[WargodHernandez]

I see one odd indentation issue with old C-style comments /** when \param spans several lines

This is referring to how it merged some \param or \return values into a paragraph rather than line by line? its not a deal breaker for me, but I will say that those style comments are harder to read like that.

[WargodHernandez]

I noticed that it isn't forcing braces around single statement if's. I thought we were supposed to be doing that.

for example here are lines 94-98 of DistanceComputationTools.cpp after the clang format

				for ( std::size_t i = 0; i < perCellTriangleList.totalCellCount(); ++i, ++data )
				{
					if ( *data )
						delete ( *data );
				}

[asmaloney]

I will say that those style comments are harder to read like that.

Agreed. Let me look into it again. The "correct solution" is to use proper C++-style comments...

[asmaloney]

I noticed that it isn't forcing braces around single statement ifs

clang-format cannot do that AFAIK. That requires clang-tidy.

[WargodHernandez]

I see one odd indentation issue with old C-style comments /** when \param spans several lines

This is referring to how it merged some \param or \return values into a paragraph rather than line by line? its not a deal breaker for me, but I will say that those style comments are harder to read like that.

Could we set CommentPragmas to include tests for \param and \return to exclude the comment reflow?

[asmaloney]

(see draft PR :-) )

[WargodHernandez]

What about assignment alignment (example from DistanceComputationTools.cpp) before the format

        GenericIndexedCloudPersist* referenceCloud		        = reinterpret_cast<GenericIndexedCloudPersist*>(additionalParameters[0]);
	const DgmOctree* referenceOctree				= reinterpret_cast<DgmOctree*>(additionalParameters[1]);
	Cloud2CloudDistanceComputationParams* params	                = reinterpret_cast<Cloud2CloudDistanceComputationParams*>(additionalParameters[2]);
	const double* maxSearchSquareDistd				= reinterpret_cast<double*>(additionalParameters[3]);
	bool computeSplitDistances				        = *reinterpret_cast<bool*>(additionalParameters[4]);

after the clang format

GenericIndexedCloudPersist* referenceCloud =
		reinterpret_cast<GenericIndexedCloudPersist*>( additionalParameters[0] );
	const DgmOctree* referenceOctree = reinterpret_cast<DgmOctree*>( additionalParameters[1] );
	Cloud2CloudDistanceComputationParams* params =
		reinterpret_cast<Cloud2CloudDistanceComputationParams*>( additionalParameters[2] );
	const double* maxSearchSquareDistd = reinterpret_cast<double*>( additionalParameters[3] );
	bool computeSplitDistances = *reinterpret_cast<bool*>( additionalParameters[4] );

[asmaloney]

So the first thing is that looking at spacing on GitHub is problematic when using tabs. Here's what that section looks like in-editor:

The second thing - the reason two of assignments wrap is because of line length which I have set to 110. I would prefer it even lower, but that was the setting that disrupted the least yet still makes it more reasonable to work with the code in an editor.

(This could be solved and made more readable if we would use auto for reinterpret_casts like this :-) )

[WargodHernandez]

my final issue that I've seen is for function definitions again see DistanceComputationTools.cpp

Before clang

DistanceComputationTools::SOReturnCode
DistanceComputationTools::synchronizeOctrees(	GenericIndexedCloudPersist* comparedCloud,
												GenericIndexedCloudPersist* referenceCloud,
												DgmOctree* &comparedOctree,
												DgmOctree* &referenceOctree,
												PointCoordinateType maxDist,
												GenericProgressCallback* progressCb/*=0*/)

After

DistanceComputationTools::SOReturnCode DistanceComputationTools::synchronizeOctrees(
	GenericIndexedCloudPersist* comparedCloud, GenericIndexedCloudPersist* referenceCloud,
	DgmOctree*& comparedOctree, DgmOctree*& referenceOctree, PointCoordinateType maxDist,
	GenericProgressCallback* progressCb /*=0*/ )

are we sure we want BinPackParameters: true

[WargodHernandez]

So the first thing is that looking at spacing on GitHub is problematic when using tabs. Here's what that section looks like in-editor: Yeah I am looking at it in VS2019 locally, just copied and pasted to GitHub as text rather than screenshot

[asmaloney]

Ah! It's missing the first TAB - gotcha.

[asmaloney]

are we sure we want BinPackParameters: true

Indeed it looks better in most cases. Nice one!

[WargodHernandez]

since we have a lot of instances of assignment being aligned in the code base already would AlignConsecutiveAssignments: true make sense or did you try that and see other issue popup from it?

[asmaloney]

I find it can be problematic because of line length and it can do goofy things with function parameters with default args.

Personally I find that style more difficult to read most of the time.

(I feel about a 7.5/10 against this :-) )

Give it a try and see what you think.

[WargodHernandez]

I'd say in areas where assignments aren't multi-line and the LHS variables are similar in length, I prefer the alignment. I don't like the alignment when one or two variables cause a massive white space between the LHS and RHS.

I concede that it would probably be a bigger pain than it would be worth.

[WargodHernandez]

I think if we go through with this we will want to do a clang tidy for braces on the single line ifs, with the length limit causing some line splits it makes some single statement ifs multi line.

[asmaloney]

OK - good point. I'll look at doing that and making a separate PR for it.

[asmaloney]

It turns out we have a bit of a chicken and egg problem. I can add the braces, but they will automatically be inserted like this:

There are over 500 cases, so I'd rather not go through and fix by hand.

So if we are going to go that route I'll do a PR for it right before we add clang-format so it gets formatted immediately.