RFC Coding Guidelines: Formatting Class Member Declarations | Haiku Project

[cb88]

It is possible that is going to occur anyway… since new patches going into gerrit are expected to be required to update to the new format and they may conflict with existing patches… I guess that just goes to show its not good to leave patches sitting around too long getting stale in gerrit.

But of course a global change would cause more of that. So, that leads to existing patches should reformat as they are put in and new patches should be written that way from the start, and at some point a global reformat is done when few patches are in flight that haven’t accounted for this.

But then you have to ask… this is very complicated so might be better to just bite the bullet and deal with the stale patches by rebasing them after the format update.

[nielx]

I have tried playing around with the configuration a bit more, and I found AlignConsecutiveDeclarations. If this is enabled, the BHandler example for the article is reformatted as follows:

class BHandler : public BArchivable
{
public:
	BHandler(const char* name = NULL);
	virtual ~BHandler();

	// BHandler guts.
	virtual void MessageReceived(BMessage* message);
	BLooper*	 Looper() const;
	const char*	 Name() const;

	// Fictional very long member function declaration
	status_t	 Launch(const entry_ref* ref, const BMessage* initialMessage = NULL,
			team_id* _appTeam = NULL) const volatile noexcept;

private:
	typedef BArchivable _inherited;
	friend inline int32 _get_object_token_(const BHandler*);
	friend class BLooper;

	int32 fToken;
	char* fName;
};

Observations from this example and after running it on Window.h:

• The constructor and destructor do not have return types and are thus not aligned with other members
• With the configuration as given, each block demarcated by public, protected or private will have it’s own alignment calculations.
• The style is very similar to what we have right now, and we can describe the rules so that the formatting can be applied manually and automatically.
• Running it over Window.h gives some strange results (i.e. suddenly a new block starts and the alignment changes without any reason), but that must be a bug so that can be fixed.

The style can be manually applied by adding a style instruction to haiku-format like:

 haiku-format -style="{BasedOnStyle: Haiku, AlignConsecutiveDeclarations: {Enabled: true, AcrossEmptyLines: true, AcrossComments: true}} FILENAME"

[refaQtor]

I’m a proponent of run the tool over the whole tree and commit all the reformatting all at once. and, yes, then all the pending patches will need to run the reformatter, too. I don’t get adding all the effort of opt-in/out. any manual intervention for formatting seems wasted time (our very limited resource). any “need” for custom format that cannot be done automatically by a formatting tools is highly suspect, in my mind.

this whole thread is long overdue. this issue alone is what has kept me from bothering to post fixes and updates that I’ve done. My first commit, years ago now, a relatively small one from the low-hanging fruit recommended on the tickets, took three volleys over a week to get accepted… entirely for formatting issues that the, then, incomplete fromatter didn’t handle.

anyway, I hope this shakes out to alleviate that issue. I’ve got some pent up code desire to fix/advance the native Haiku environment.

keep up the great work, everyone!

[axeld]

My two cents on this topic:

• In general, I hate it, if I have to adapt my preferences or taste because of an inferior technical solution. That’s actually one reason I got involved with software development in the first place.
• Unfortunately, I am now old enough to admit that I won’t be able to solve all those problems, so sometimes, compromises must be made.
• I really like the way we structure our headers, and having written a ton of them myself, I know that it’s really simple to do, and problems with it are actually rare at the moment.
• If this would just be about new C++ features, I’d say let’s wait and see how it’ll go.
• However, it’s not all this is about, as Pulkomandy nicely pointed out: it would be great never having to worry about the coding style ever again.
• I know for a fact that many aspects of a coding style are just subjective preferences, and you can get used to pretty much everything, even that ugly style I have to use at work. But I still find it ugly.
• I’m very sceptical about automatic formatting tools, though, as I haven’t seen one that works flawlessly. Even if they just follow rules, they tend to mess things up, and, since they don’t really understand the semantics that may be behind line breaks and indentations, often choose bad line breaks, for example.
• I would be against throwing such a tool at our whole codebase at once. For one, see my last point, and secondly, losing all patches in Gerrit would be a show stopper for me. The tool should improve our work flow, and not throw everything away. But maybe it would be possible to automatically convert them, too.
• I am not convinced of the necessity of a change given the issues it will introduce. If all of this would work out okay in some way, though, it would be a great addition.
• If we will change the class definition style, I think we should simply use what we had before: single tab at the beginning, and no tabs after. That’s really common, too. I’m not too fond of new experiments like what Pulkomandy suggested. I would also put the curly braces at the end of the line after “class”.
• But if we really will change the style, I’d have another suggestion: to switch to 100 columns always, and always indent the next line two tabs, not just one.

[waddlesplash]

@nielx

the BHandler example for the article is reformatted as follows:

This actually looks not too bad. If it added just one more (“extra”) tab between type and name, rather than just aligning based on the longest “prefix”, it would look not too far off from our current style. I wouldn’t so much mind changing to per-block alignment, I think.

@axeld

I’m very sceptical about automatic formatting tools, though, as I haven’t seen one that works flawlessly. Even if they just follow rules, they tend to mess things up, and, since they don’t really understand the semantics that may be behind line breaks and indentations, often choose bad line breaks, for example.

Agreed. Code formatters may do 90% of the job, and may be especially useful for new contributors, but there will always be nuances they won’t be able to get quite right.

@axeld

I would be against throwing such a tool at our whole codebase at once. For one, see my last point, and secondly, losing all patches in Gerrit would be a show stopper for me. The tool should improve our work flow, and not throw everything away. But maybe it would be possible to automatically convert them, too.

Also very much agreed.

[nielx]

So I thought it is about time to make up the balance and see where the discussion is heading. I think there are three threads of discussion going on, so I will try to group them as such.

1. What role does a formatting tool like haiku-format have in our community?

While the question is not an explicit part of the original RFC, it is worth reviewing where we stand. @PulkoMandy writes that in his day job, the use of clang-format is mandatory and its automated formatting saves a lot of time that could be spent on other aspects of code review. He states that the trade-off with adjusting the style to the capability is worth it. @waddlesplash is suggesting that instead of changing our standards, we can switch off haiku-format inside class definitions, though there is a bit if discussion on whether that then does not defeat the purpose of using it, as it the tool will now only partially doing what it should. @axeld writes that the idea of a tool is great, but that he is sceptical if formatting tools in general as he thinks there may be too many issues with them.

My position is that haiku-format could and should be an integral tool in our development process, as it will save time and increase the consistency of our code base. I recognize that this will sometimes lead to trade-offs, like haiku-format distributing arguments over multiple lines following the rules, but a human would add an extra break for visual clarity. But in the end the value of consistency should prevail.

2. What is the replacement style?

In the article I proposed going for the out of the box style that haiku-format currently wants to use, which I dubbed Normalized Class Member Declaration Style. @apl approved of this style. @axeld wrote that if we were to change the style, this would make most sense to him too. And I happen to know that @X512 also supports this style.

@PulkoMandy proposed an alternative two-line function definition style, which I (somewhat) managed to configure in haiku-format, though with the downside that those rules would also impact function definitions outside of classes.

I have also experimented with the tool’s ability to align declarations, which gave decent results, though I did run into a bug when I used it on Window.h, so that might need some time. It also would apply outside of class definitions, which needs to be discussed (or the tool needs to be altered).

My position is that I think the originally proposed style is still the best, because:

1. The readability is fine. It is not as nice as the table view, but as the RFC states, there are times where the table view is not nice either. Furthermore, code highlighting should be able to add visual clues to the class members. I use Visual Code with IntelliSence C++, and the method names may not align, but they are all nicely marked in gold. Even in the Discourse formatter on this forum, you can see the formatter finds the member names.
2. The format is easy to write by hand, with a large chance you will get it right the first time. This is also the case for PulkoMandy’s proposal, but it is not the case for my attempt to align declarations. That may require a reformat when members are added or deleted.
3. The style change does not require any additional modifications to clang-format.

I could go for the aligned declarations too, with the point that we use it out of the box and that it therefore also applies to function and variable declarations outside of the class. Interestingly enough, OS.h already seems to (mostly) do that, though scheduler.h uses the normalized declaration style.

3. How to implement the style if we agree to change it?

This discussion was (unexpectedly) also very much alive. My proposal was to enforce it for new changes, with the possibility for the contributor to choose to exclude it from the new formatting rules and to keep it under the old formatting rules.

@fest3er seems to agree with that position.

@PulkoMandy takes the position that there should not be an exception. Furthermore, he argues that we should eventually do a big reformat on the tree. @refaQtor agrees. @X512 points out that a big bang refactor will invalidate changes on Gerrit, which @cb88 acknowledges, though comes out on the side of biting the bullet. @axeld would rather not do a big bang because of the patches on Gerrit, which is shared by @waddlesplash .

My position is that I have been swayed that keeping an opt-out for the legacy style might be confusing, so when I revise the RFC, I will remove that option. I will punt on the question whether or not a big bang rewrite is in order: that can be the topic for another RFC. For now, I will only propose it applies to contributions after point in time to be determined.

What’s Next

Reading the discussion, while I think there is a bit of scepticism on whether clang-format will really work well under all situations, I get the sense that we are willing to try.

What is less clear is the desired style. Are we going for the Normalized Class Member Declaration Style (see RFC), or for the Aligned Declaration Style. Maybe a poll to see what the distribution is?

[PulkoMandy]

@nielx

Maybe a poll to see what the distribution is?

This is not going to work. I think if you do a poll, the likely winner is “patch haiku-format to implement the current style”. Poll results do not care about wether someone is actually willing to write that patch.

Or, you can remove that option from the poll, but then, what if someone later implements the needed changes in haiku-format? Do we revert back to the old style then?

There are in fact multiple cuestions here, not all of which can be answered by a poll:

• what is the exact definition of the current style,
• wether this format can be implemented by a bot,
• independently of that, is one of the other proposed formats actually better, in two ways: easier to write (which is less useful if there is an automated tool to do it), and/or easier to read

Furthermore, we now have several different proposed styles:

• the existing one in the current guidelines
• your proposed “normalized” one
• my proposal with newlines instead of tabs
• approximations of either the current style or my “newlines” version

If it gets to a vote, should it be a ranking of these four with Condorcet/Borda count? Should we do this vote independently of the technical possibility of automating the formatting?

And then, we don’t even prioritiz these things the same way. Some people (like me) think that automated formatting is more important than readability. Some think the important thing is making the new style easier to write manually, even if readability is not as good. So I’m not sure how we can answer all of these questions with a vote?

@nielx

@axeld would rather not do a big bang because of the patches on

That’s a valid concern, however, if there is an automatic tool, I think it is easy to update these patches:

• rebase the patch, keeping the old formatting whenever there are conflicts
• reapply the automatic formatting tool

This finally leaves the question of wether there should be an opt-out to preserve the old formatting, and what rules should be applied. This is of course not needed if the new style is close enough to the old one (then there would be only some slight reformatting).

From the viewpoint of someone trying to send a patch to Haiku, we have the following outcomes:

1. the code was mass-reformatted:

you just need to run the formatting tool on your changes. There will not be any problems with coding style

2. the code was not mass-reformatted:

you can try to make changes following the existing code style (which will not be documented anymore in the coding guidelines, so you have to guess what it was). Then we have two options:

• you need to add clang-format disable markers around the part of the code that is not reformatted and you don’t want to change, or
• you can’t use the formatting tool on any other parts of the code either, because it would also reformat this to the new style

or alternately, you decide to reformat the code in all “parts” of the code that you touch. It will be difficult to do this with an automated tool at a fine granularity, so here “part” will probably be an entire .h or .cpp file (or is there a better way?). In this case, code reviewers will ask that the reformatting is done as a separate commit (this is already what we do). Here again, we ask contributors to do something nontrivial and not so easy to automate.

I think you can see how this option 2 (in all its variants) maintains or creates friction on code reviews over coding style, which can be entirely removed if the code is reformatted.

Finally, regarding the “loss” of line ownership (git blame attribution). Personally I end up looking at git log -p rather than git blame to understand the history of a file. So I don’t think that this is extremely important. In any case, I think it is less important than making new contributions easier and having a consistent style.

That being said, if there is enough opposition to this, I will accept that we move gradually to an automated formatting, rather than staying on a manual one.

[X512]

I think that code mass reformat is not needed, only source files touched by change are needed to be reformatted.

[nielx]

@PulkoMandy

There are in fact multiple cuestions here, not all of which can be answered by a poll:

• what is the exact definition of the current style,
• wether this format can be implemented by a bot,
• independently of that, is one of the other proposed formats actually better, in two ways: easier to write (which is less useful if there is an automated tool to do it), and/or easier to read

Ugh, yeah, you’re right in pointing out this is a bit of a puzzle.

This is what I want to accomplish:

• Over the past few years we have spent time and resources getting haiku-format to implement most of our style.
• My intention is to ‘finish’ the projects started by those before, by making it cover as much of our coding style as possible, and having a bot available that checks Gerrit patches.
• My red line is that I do not want to customize clang-format further, with the exception of implementing the configuration for haiku-format.

@PulkoMandy

Furthermore, we now have several different proposed styles:

• the existing one in the current guidelines
• your proposed “normalized” one
• my proposal with newlines instead of tabs
• approximations of either the current style or my “newlines” version

With this in mind, the existing one is a no go, as this would require modifying clang-format. I consider your proposal not viable either, because the configurations in clang-format required to approach it, would also affect function definitions, thus breaking other parts of the coding style.

This leaves the normalized style, or the aligned declaration style. I guess that’s where the poll comes in. I currently have a preference for the normalized style, but I am fine codifying either style. I think that’s where I need a bit of feedback before I submit my final RFC to a vote.

At that point, it is up to the core developers: either accept or reject it. If it is rejected, then I will drop the project of the haiku format bot (no hard feelings!), and we can go on with the rest of our work.

[nielx]

Just a thought, maybe that’s the final approach. I can update the RFC to display the two viable outcomes, and then the core team vote can be the following four choices

• Accept proposal; normalized style
• Accept proposal; aligned declarations
• Reject proposal; if accepted, normalized style
• Reject proposal; if accepted, aligned declarations

[Munchausen]

We use uncrustify at work as we found it to be far more flexible than clang format (or other options).

[squizzler]

Edit: I am not sure why the following is felt to be off-topic, perhaps rather flippantly expressed. My point was that the conversation showed that even those who thought they were familiar with the OP’s abbreviation can be mistaken and demonstrated that it is worth defining abbreviations.

I rest my case as to why it might be helpful if posters did not assume everyone knew abbreviations, even if apparently familiar in the scene.

[waddlesplash]

@nielx

like haiku-format distributing arguments over multiple lines following the rules, but a human would add an extra break for visual clarity. But in the end the value of consistency should prevail.

If haiku-format can’t even be “fuzzy” about linebreaks (i.e. made before they’re strictly necessary, but not too much far before), that is a much stronger objection to it in my view than a lot of others proposed here. Method definitions aren’t the only place we do this sort of thing, either.

In other words: having haiku-format especially for new contributors, or old contributors who aren’t in practice with the coding style or simply don’t want to spend more time formatting than they must, great. But if the tool is to become the “single source of truth”, and no deviations from whatever it spits out are permitted, well, I am firmly against that and expect to remain so.

This is source code, not machine code. Using tools to help write it, audit it, test it, and so on is good. But source code is written by humans for other humans to read and write as much (or more, in some ways) than it is for compilers to interpret. If we’re sacrificing a major part of that flexibility in the name of “consistency”, that does not sound like a good move to me.

@nielx

with the point that we use it out of the box and that it therefore also applies to function and variable declarations outside of the class

But why? We already have a haiku-format tool here because we aren’t using out-of-the-box clang-format. Surely modifying an existing formatted that clang already provided shouldn’t be too much, since we’ve already done that for other modules here, yes?

[PulkoMandy]

@waddlesplash

In other words: having haiku-format especially for new contributors, or old contributors who aren’t in practice with the coding style or simply don’t want to spend more time formatting than they must, great. But if the tool is to become the “single source of truth”, and no deviations from whatever it spits out are permitted, well, I am firmly against that and expect to remain so.

This seems a bit theoretical. I think the comments from haiku-format usually make sense, and when they don’t, we have options to tweak to make things better.

And in very specific cases it’s always possible to insert a directive in the sourcecode to disable the automatic formatting for a section of code if you really need to (I have not yet seen a case where that would be needed in my work projects).

@waddlesplash

But why? We already have a haiku-format tool here because we aren’t using out-of-the-box clang-format. Surely modifying an existing formatted that clang already provided shouldn’t be too much, since we’ve already done that for other modules here, yes?

That is also a bit theoretical. Nielx doesn’t want to make these changes to haiku-format. We end up with basically two teams of people here:

• Those who think the output of haiku-format is good enough even if not perfect,
• Those who think someone should improve haiku-format to match the current coding guidelines (or some improved version).

Of course, no one in the first group is going to put in work to improve haiku-format. They think there are more important things to work on. And, in the second group, there are people who think formatting code by hand is a better option than fixing haiku-format. So there is only a subset of that group (possibly empty) of people who will actually fix the problem in haiku-format.

So, we can vote for a decision of “someone should fix haiku-format before we start using it more widely”. This has been the ongoing situation for a few years. As a result, we spend a lot of time finding and fixing coding style problems in code reviews, people try to use haiku-format and it makes their code worse instead of better. And no progress is made in any direction.

How do we get out of this hole? Who is going to fix haiku-format? Or can we get out of this in the other way: first set up haiku-format, and then improve it gradually when we find that the formatting isn’t appropriate.

[humdinger]

This thread intends to have a technical discussion by the Haiku devs and (to a lesser extend) code contributers about the future of Haiku’s code style. I’ve marked distracting posts as off-topic. People are free to open new threads in the off- or an on-topic category.

[owenca]

@nielx

Technically, it is not impossible to teach clang-format to apply our rules, as demonstrated by the GSoC 2021 patch that implements Haiku’s member formatting rules.

If anything, that patch only demonstrated the opposite because it basically doesn’t work. (See here.) So if anyone’s interested in giving it another shot, it would make sense to start fresh. However, as I stated in a previous email exchange with Niels, “I don’t think clang-format can/should be customized for this, or else I probably would’ve done it.” I said this only after having worked on customization contracts and contributed hundreds of patches to clang-format alone.

Getting haiku-format off the shelf and putting it in use on a bigger scale, unlocks the value of the tool, and the value of the ideas created during GSoC 2021.

IIRC the GSoC 2021 project was an attempt at customizing clang-format from scratch despite the existence of haiku-format (probably due to some misunderstanding/miscommunication). As a result, the student only worked on issues already covered by haiku-format or fixed upstream in clang-format (plus a couple of new features that had very little hope of success IMO). This is why nothing from the student’s work was merged into haiku-format.

[owenca]

@nielx

@Zakero

I didn’t see anything for function attributes, such as [[nodiscard]] virtual bool DoesThingExist();.

Should any comments be added about their use?

Thanks for reminding me of this! [[nodiscard]] is great and indeed cannot be accounted for at all in the current table-formatting style.

There’s also explicit. For attributes though, clang-format has the BreakAfterAttributes option.

[owenca]

@PulkoMandy

In my paid work project, where there is no time for perfect solutions because it’s a commercial thing, we set up a coding style that is acceptable given clang-format constraints. Then we have it integrated in our buildsystem, everyone just runs the “fix-format” script before pushing their changes for review, and there is 0 time spent in code review for style fixes, because whatever the tool decides to do is right, by definition.

This saves a ton of time. Then this time can be used for other code review aspects: reviewing the overall architecture of the code, checking for bugs, making sure the documentation is updated as needed, and so on.

I couldn’t agree more!

[PulkoMandy]

@owenca

IIRC the GSoC 2021 project was an attempt at customizing clang-format from scratch despite the existence of haiku-format (probably due to some misunderstanding/miscommunication).

That’s not true. There was indeed some miscommunication, but the project started from existing haiku-format sources and known problems with it.

There were several false starts and experiments in attempting to rebase haiku-format on top of a newer version of clang-format, which caused some regressions in the way it was done. Later in the project, and following your input, this approach was abandoned and some of the patches were moved back to haiku-format, keeping it on the much older clang-format version it was based on at the time.

And there were also difficulties because I mentored the project while having no idea how the clang-format codebase works. I take note of that, and if we do such a project again we will make sure to have someone who knows the involved codebase doing at least part of the mentoring.

In the end, indeed, the code was not clean enough and not working well enough to be usable. On its own that doesn’t prove anything on wether it is possible or not. Someone else giving it a try may be more successful. But then there’s the question of wether it can be done in a way that fit clang-format design, could be upstreamed there, or maybe if we should consider other tools. Or, maybe, we can make some changes to the coding style.

[owenca]

@PulkoMandy

@owenca

IIRC the GSoC 2021 project was an attempt at customizing clang-format from scratch despite the existence of haiku-format (probably due to some misunderstanding/miscommunication).

That’s not true. There was indeed some miscommunication, but the project started from existing haiku-format sources and known problems with it.

Ah, so I remembered it wrong! In fact, GSoC 2021 started on an obsolete version of haiku-format (version 6) despite the existence of a new version (version 10) released months earlier.

There were several false starts and experiments in attempting to rebase haiku-format on top of a newer version of clang-format, which caused some regressions in the way it was done. Later in the project, and following your input, this approach was abandoned and some of the patches were moved back to haiku-format, keeping it on the much older clang-format version it was based on at the time.

So it seems that the student was always using haiku-format 6 as a baseline. That would explain why the student kept working on issues already addressed in haiku-format 10.