Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposed clang-format file #22

Open
asmaloney opened this issue Apr 24, 2020 · 21 comments
Open

Proposed clang-format file #22

asmaloney opened this issue Apr 24, 2020 · 21 comments
Labels
enhancement New feature or request

Comments

@asmaloney
Copy link
Member

asmaloney commented Apr 24, 2020

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 asmaloney added the enhancement New feature or request label Apr 24, 2020
@asmaloney
Copy link
Member Author

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
Copy link
Collaborator

I'd appreciate a branch to test it out 👍

asmaloney added a commit that referenced this issue May 17, 2020
@asmaloney
Copy link
Member Author

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Member Author

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
Copy link
Member Author

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

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

@WargodHernandez
Copy link
Collaborator

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
Copy link
Member Author

(see draft PR :-) )

@WargodHernandez
Copy link
Collaborator

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
Copy link
Member Author

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:

Screen Shot 2020-05-20 at 17 55 03 PM

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Member Author

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

@asmaloney
Copy link
Member Author

are we sure we want BinPackParameters: true

Indeed it looks better in most cases. Nice one!

@WargodHernandez
Copy link
Collaborator

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
Copy link
Member Author

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.

Screen Shot 2020-05-20 at 18 12 13 PM

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

Give it a try and see what you think.

@WargodHernandez
Copy link
Collaborator

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
Copy link
Collaborator

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.

image
image

@asmaloney
Copy link
Member Author

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

@asmaloney
Copy link
Member Author

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:

Screen Shot 2020-05-20 at 19 34 00 PM

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants