The big downside is that very troublesome to do all that table formatting by hand. You will forget what you attempted to write.
Thanks for reminding me of this! [[nodiscard]]
is great and indeed cannot be accounted for at all in the current table-formatting style.
Agreed, and like I acknowledged in my article, when the class definition does not have too many frills, it looks great!
An aspect I did not touch on in the article though, is that I also think the development tools have improved a lot since the time the style was introduced. Code editors have gotten a lot better at highlighting. IDEs with code completion can go a long way in helping you find methods you need without having to open the header. Even the code formatting in the article illustrates how color and font choice help with reading the function declaration. Furthermore, we have the Haiku Book which quite literally puts all the methods a table (see BApplication as an example).
If I were to describe the style into plain English, I’d say we were going for the rules that for class member function declarations we (1) always want to break after the return type and (2) indent all lines by one extra tab against the baseline, except for the first line of the declaration. Note that my rule 2 would not indent the constructor and a non-virtual destructor.
For pragmatic reasons, I am looking at the options that are currently in clang-format
to see what we can do to come closer to the proposed style. As per the economic argument, I’d rather use something that already exists rather than modifying the clang-format
where possible.
For rule 1 (always want to break after the return type), there is the aptly named AlwaysBreakAfterReturnType option. It is currently set to TopLevelDefinitions
to get the desired style for function definitions according to our style guides. I think we would then go for the configuration All
in order to accomplish this, though the side effect is that this would also apply for function declarations outside of classes and structs.
For rule 2 (extra indention of subsequent lines), I think the one that we should use is IndentWrappedFunctionNames, which is currently set to false
.
Calling haiku-format -style="{BasedOnStyle: Haiku, SpaceAfterCStyleCast: false, IndentWrappedFunctionNames: true, AlwaysBreakAfterReturnType: All}"
then results in:
class BHandler : public BArchivable
{
public:
// The constructor doesn't have a return type and so it looks a bit strange. Is this a
// problem?
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
// The parameters on next lines get indented one step further
status_t
Launch(const entry_ref* ref, const BMessage* initialMessage = NULL,
team_id* _appTeam = NULL) const volatile noexcept;
// NOTE: compared to the example in the article, I have separates the private methods
// from the private members. I think this is not part of the changes being discussed here,
// in both the current and new style, these should be kept separated
private:
friend inline int32
_get_object_token_(const BHandler*);
private:
// Do we want newlines also in the fields between the type and the name? or is it not
// needed?
typedef BArchivable _inherited;
friend class BLooper;
int32 fToken;
char* fName;
};
// Unintended side-effect: also definitions outside class declarations are affeced by
// IndentWrappedFunctionNames
void
Foo::FooFunction()
{
}
So clang-format
can somewhat accommodate these style changes, though it does not support making an exception for a break after a return type for non-member function declarations (which I am not sure is that bad), and it indents the function names also outside of class definitions (which I think is a departure of our current style).
The aim of using a code formatter is to introduce and maintain consistency in a code base, which is ultimately the aim of our coding guidelines as well. My assumption here is that using haiku-format
will increase the consistency in the code base, which I think is evidenced by what I am seeing running it over the changes, even if it needs a little bit more tweaking.
If we then exclude class definitions from the scope of haiku-format
, we would then exclude a non-trivial part of the code and would be counter to that aim. I am on the line of thinking as PulkoMandy, which is that ultimately the value of the code formatter outweighs dropping style preferences it does not support.
Note that I genuine think there are objective and subjective reasons to change the formatting style of class members, so I am tip-toeing around the discussion on whether automated formatting tools are the goal by and for itself here.
The proposal outlines the rule changes, which includes an opting into the old style for now if that’s preferred (or less bothersome than reformatting a big class). I do use the term opting in deliberately here, because I do want to propose adding the // clang-format off: legacy class member formatting
explicitly to code that opts into the legacy style.
I don’t think that current table formatting actually makes definitions easier to read. Maybe an opposite. It is a matter of personal taste.
Not sure I agree with that (but also not sure if I understand what you say exactly).
Basically we are balancing several costs and benefits here:
- The cost of developping a tool to format code exactly as we do it now (which is complex, so high cost)
- The cost of manually formatting and manually reviewing other people’s change for formatting problems (also relatively high, but more spread out over time)
- The benefit of an easily readable format where return type, qualifiers, and method names are neatly separated (sure, it is a little subjective)
- The cost of maintaining two coding styles in parallel and migrating from one to the other, either in one big “reformat everything” over a few weeks, or in a slower and more manual way
- The cost of change: it is already “paid” by us all spending time in this discussion. If the new formatting is automated, there is not so much time wasted “learning” the new style, we can hopefully soon let haiku-format handle it for us.
I think once the new formatting is decided, we should rather aim to migrate quickly. It seems not so hard to run haiku-format on, say, headers/os, and make one big commit that reformats everything to the new style (or maybe split it into one commit for each subdirectory of headers/os). Then, it is done for a large majority of class declarations in the codebase. We then have a single new style again and we can move on. For me that is better than allowing both the old and new styles to co-exist for a possibly very long time.
Global reformatting have an issue that it will invalidate many pending patches in Gerrit.
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.
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"
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!
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.
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.
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.
Also very much agreed.
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:
- 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.
- 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.
- 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?
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?
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:
- 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
- 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.
I think that code mass reformat is not needed, only source files touched by change are needed to be reformatted.
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
.
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.
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
We use uncrustify at work as we found it to be far more flexible than clang format (or other options).
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.
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.
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?