RFC Coding Guidelines: Formatting Class Member Declarations | Haiku Project

[PulkoMandy]

@X512

It also dictate 2 blank lines between each function, but in my personal C++ code style I usually use one blank line and 2 blank lines to separate function groups.

In Haiku we use two blank lines between functions and #pragma mark to separate function groups (possibly in a comment because compilers don’t implement this pragma anymore, sadly).

In GCC they use an ASCII page break character between function groups, so if you print out the code, it will start each group on a new page.

I’m not usre what your point is here. There are different coding styles in different projects? Yes, we already knew that.

And yes, the coding style is strict. That’s the point of having a coding style. Having all the code formatted the same way makes it easier to read if you are used to this style. Any deviation from it requires extra effort to understand the code. Of course, if you are used to another style, you think the opposite is true. There is no perfect solution here.

@nielx

I think this really is one of the fundamental debates, and there is no one true answer.

There isn’t, but there are compromises possible. In the case of clang-format, if you find the automated formatting not satisfying, you can use some markers in the sourcecode (clang-format-disable) to disable it and have it ignore these sections of the code. If you do this a lot, it will surely raise some eyebrows from the reviewers. But if you do it sparsely, I think it is acceptable. We can also periodically look into these and see if we need to tweak some clang-format settings.

@owenca

So the Aligned Declaration Style doesn’t seem to be a viable option.

Well that’s unfortunate. In that case, the automated formatting degrades readability. I’m not sure I’m willing to compromise readability so much to automated formatting…

[X512]

@PulkoMandy

And yes, the coding style is strict. That’s the point of having a coding style.

It is my objection to Waddlesplash’s opinion:

@waddlesplash

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.

There are no “flexibility for human” in current Haiku style. It is strict in the same way as clang-format style. Differences are only in personal sense of taste, it is not objectively better in any way comparing to clang-format. Both styles do not allow case-by-case formatting to improve readability.

Examples of potential case-by case formatting to improve readability:

	if ((attributes & B_USER_PROTECTION) != 0) {
		pte->flags |= (1 << pteUser);
		if ((attributes & B_READ_AREA)    != 0) pte->flags |= (1 << pteRead);
		if ((attributes & B_WRITE_AREA)   != 0) pte->flags |= (1 << pteWrite);
		if ((attributes & B_EXECUTE_AREA) != 0) pte->flags |= (1 << pteExec);
	} else {
		if ((attributes & B_KERNEL_READ_AREA)    != 0) pte->flags |= (1 << pteRead);
		if ((attributes & B_KERNEL_WRITE_AREA)   != 0) pte->flags |= (1 << pteWrite);
		if ((attributes & B_KERNEL_EXECUTE_AREA) != 0) pte->flags |= (1 << pteExec);
	}

void KeyboardHandler::KeyString(uint32 code, char *str, size_t len)
{
	uint32 i;
	char *ch;
	switch (modifiers & (B_SHIFT_KEY | B_CONTROL_KEY | B_OPTION_KEY | B_CAPS_LOCK)) {
		case B_OPTION_KEY | B_CAPS_LOCK | B_SHIFT_KEY: ch = chars.Get() + keyMap->option_caps_shift_map[code]; break;
		case B_OPTION_KEY | B_CAPS_LOCK:               ch = chars.Get() + keyMap->option_caps_map[code];       break;
		case B_OPTION_KEY | B_SHIFT_KEY:               ch = chars.Get() + keyMap->option_shift_map[code];      break;
		case B_OPTION_KEY:                             ch = chars.Get() + keyMap->option_map[code];            break;
		case B_CAPS_LOCK  | B_SHIFT_KEY:               ch = chars.Get() + keyMap->caps_shift_map[code];        break;
		case B_CAPS_LOCK:                              ch = chars.Get() + keyMap->caps_map[code];              break;
		case B_SHIFT_KEY:                              ch = chars.Get() + keyMap->shift_map[code];             break;
		default:
			if (modifiers & B_CONTROL_KEY)               ch = chars.Get() + keyMap->control_map[code];
			else                                         ch = chars.Get() + keyMap->normal_map[code];
	}
	if (len > 0) {
		for (i = 0; (i < (uint32)ch[0]) && (i < len-1); ++i)
			str[i] = ch[i+1];
		str[i] = '\0';
	}
}

[PulkoMandy]

In the Haiku style bot of these would require newlines after the case or the clising parenthese of the if. The code after the linebreak will then be properly aligned, and it won’t be so wide that it can’t fit on screen (especially if using multiple columns, for example in a side-by-side diff or if you just like to have multiple files visible at once).

It is not perfect, but it gives good enough results. And yes, I think it really helps that all the code is formatted the same.

You could make the same argument about syntax highlighting. Do you think that people should edit their code in StyledEdit and manually apply formatting? They could colorize different parts of the code differently, put really important code in bold or larger text, and so on. But, soon enough, you want some common rules so that all the code looks the same. And maybe you want some way to personalize it.

For synrax highlighting, this seems a strange question to ask. Why is it different for indentation?

And again, as I said, with or without tool, deviations from the style could be allowed. But there has to be a really good reason for it. I am not convinced by your example. I have to think about the syntax of the code a lot more. For example here is how I read your second example: my first thought is, is the break in that swich/case missing? Ah no, it’s there, but it’s in a strange place on the right. While thinking about that, I lose track of the semantics.

After years and years of reading code always formatted the same way, I am trained to subconsciously parse it. But this training is not as good as that of a C++ compiler, and I rely a lot on the indentation, the variable naming convention, etc. This allows me to navigate the codebase and understand the code quickly. Everything different from that convention disrupts that flow and requires me to switch to a slower, more careful parsing of the code to understand it.

Again, I am not absolutely opposed to deviations. But it has to be really, really worth that extra effort. In your examples, personally I think it isn’t.

[X512]

@PulkoMandy

You could make the same argument about syntax highlighting. Do you think that people should edit their code in StyledEdit and manually apply formatting?

It is literally done in this way for many Oberon dialects (I have a lot of experience of working with them). Source code is stored in special binary format that support formatting. This approach have some advantages, for example mark some code parts with a different color or insert illustrations directly in source code. Biggest disadvantage is a bad integration with source version control systems such as Git.

See: GitHub - X547/BlackBox-Haiku: BlackBox Component Builder port to Haiku operating system.

[nielx]

@PulkoMandy

@owenca

So the Aligned Declaration Style doesn’t seem to be a viable option.

Well that’s unfortunate. In that case, the automated formatting degrades readability. I’m not sure I’m willing to compromise readability so much to automated formatting…

While I have explicitly taken the position to only suggest styles that are supported by clang-format, I would not rule this out because of bugs. It is a valid configuration flag, so if this is your preferred style, then I’d be more than happy to raise issues with the llvm project, and maybe even look into if it is fixable myself. I just don’t want to develop/maintain customizations.

[nielx]

FYI: the proposal has been reviewed on the haiku-development mailing list.

Based on the low number of responses, I will withdraw the proposal. This also means that I will abandon the effort to work on automated code formatting. I have archived the haiku-format-bot repository and I have marked the RFC blog post as withdrawn.

[owenca]

That’s a shame. I’ll hold off upgrading haiku-format then.

[PulkoMandy]

I am a bit confused by nielx decision to abandon it all. I have found the bot very useful as a way to improve code formatting, save time on code reviews (even in its current imperfect version), and also show inconsistencies between clang-format and the coding style (of which some will require improving clang-format, and some will require changing the coding style).

Even if there is no short term decision of changing the coding style, I think the availability of the bot was a net improvement, and that it should not be archived, but kept running, and we should keep improving it. I’m happy to help where I can.

[marcoapc]

I agree, the work is useful and brings many benefits to code review!

[owenca]

@PulkoMandy

I am a bit confused by nielx decision to abandon it all.

I can understand the frustration.

I have found the bot very useful as a way to improve code formatting, save time on code reviews (even in its current imperfect version), and also show inconsistencies between clang-format and the coding style (of which some will require improving clang-format, and some will require changing the coding style).

You mean haiku-format?

Even if there is no short term decision of changing the coding style, I think the availability of the bot was a net improvement, and that it should not be archived, but kept running, and we should keep improving it.

Before putting more effort into haiku-format-bot, Haiku devs must come to terms with haiku-format or at least reach a consensus on what to do with the two 2-year-old blocking issues, one of which this RFC attempted to address. Even without the bot, haiku-format can be used with git before (or after) patches are submitted on gerrit. See a demo here.

[X512]

@PulkoMandy

I am a bit confused by nielx decision to abandon it all.

It is unfortunate that many developers are still stay for current old-fashioned table based class declaration that is not seen in modern C++ projects and incompatible with new C++ features. It is also less readable IMHO. Negative responses kill motivation to do something.

[nephele]

What is that response supposed to be? You aren‘t allowed to say no on a vote because it kills motivation? Then why have a vote in the first place?

For my part I made it pretty clear why I voted no: changing this to conform to a tool that always reformats everything even if it doesn‘t know how to do it correctly isn‘t the answer. This leaves us in this all-or-nothing situation of now. All obviously doesn‘t work when the developers involved say they don‘t wish to change clang-format further.

So now instead of taking the parts that work and beeing happy with it (like the bot making some parts much easier in code review) We now pick nothing. Fun.

Also old-fashioned and modern don‘t mean anything. Those are just words to attack things you don‘t like personally.

[owenca]

@X512

It is unfortunate that many developers are still stay for current old-fashioned table based class declaration that is not seen in modern C++ projects and incompatible with new C++ features.

What’s the latest C++ standard allowed in Haiku? It’s not mentioned in the style guide.

[PulkoMandy]

@owenca

What’s the latest C++ standard allowed in Haiku? It’s not mentioned in the style guide.

It’s whatever the compiler supports. Some parts are built with gcc2 and stuck with C++98. Others are using only a modern compiler (currently gcc 13) and can use new features as needed.

@nephele

What is that response supposed to be? You aren‘t allowed to say no on a vote because it kills motivation? Then why have a vote in the first place?

I find x512 response very negative as well. And, yes, I think there should not have been a vote. It was an attempt by nielx to move fast into a conclusion in#tead of a longer discussion. I think the result is as expected: people either refused to vote or voted against changes that are not fully clear and decided yet.

I have proposed an alternate formatting that would maybe work better. There are other options as well. But in the end, we didn’t even reach that: some people even voted against having a code formatting tool at all, because it migqt occasionally break careful manual formatting. I think this is a point where the debate should continue. @waddlesplash I think that was your opinion. So I’m going to ask the question differently now: assuming we had a tool that implements the current guidelines (not because I’m pamticularly attached to them, but because I want to reduce the discussion to a more specific question that we can settle), would you all be ok with haiku-format being enforced by default, with places that need manual formatting guarded with “clang-format disable” annotations?

I think this is something we can (hopefully) all agree on, and then work towards making it a reality (either improving the tool, or tweaking/changing the coding style). This would provioe a way to move forward, even if a slow one. But if even that isn’t acceptable, we have to know now, and indeed I would then understand nielx’s frustration…

[PulkoMandy]

@owenca

You mean haiku-format?

well, yes, “improved clang-format” in this context is haiku-format, since some things may not be upstreamable.

@owenca

Before putting more effort into haiku-format-bot, Haiku devs must come to terms with haiku-format or at least reach a consensus on what to do with the two 2-year-old blocking issues, one of which this RFC attempted to address.

Before Haiku devs reach a concensus, we need a better understanding of woat is or is not possrble. I think this can be done partly by gontinuing this discussion, and partly by continuing to improve the tool.

2 years isn’t such a long time in Haiku timescale (for better or worse). I think the vote already told us that the “normalized” style isn’t going to be popular with the devs, and that there are still other topics to discuss (such as, to what extent we would be ok with less readable formatting, and with using directives to disable the formatter. These things will be required for an automated formatting tool. But until then, I think we can already have a “checker” tool, that points out possible style violations. For this usage, it may even be possible to post-process haiku-format output, and remove the things we know it gets “wrong” (not matching the current style). I think getting the tool deployed as a “checker” first will help devs getting used to it and motivated to improve it. Whereas if it stays in a “this is sort of available but no one uses it” state, no progress will be made.

[X512]

@nephele

Also old-fashioned and modern don‘t mean anything. Those are just words to attack things you don‘t like personally.

It is not only a matter of my personal opinion. Most C++ projects do not use class table formatting. C++ programmers are not familiar with it. New Haiku contributors will likely be distracted by multiple round-trips on fixing code style and there are no automatic tools that can help (because such style is so rare that nobody bother to support it).

Changing code style to more common one that is supported by formatting tools and reformatting only touched source parts is a path of least resistance.

[waddlesplash]

@PulkoMandy

There are other options as well. But in the end, we didn’t even reach that: some people even voted against having a code formatting tool at all, because it migqt occasionally break careful manual formatting. I think this is a point where the debate should continue. @waddlesplash I think that was your opinion.

If the options are “use the code formatting tool for everything, no deviations allowed”, I am firmly against that. But that’s not “voting against having a code formatting tool at all.”

Some (or many) developers and contributors may want to just write code without thinking about style, then run the formatter on their patches before submitting them. This way we can still have manual formatting contrary to what the formatter would do in many places (line breaks, comment placement, etc.) but still gain most of the benefits of having such a tool.

@PulkoMandy

would you all be ok with haiku-format being enforced by default, with places that need manual formatting guarded with “clang-format disable” annotations?

This is specifically what I would be against. Again, if it’s used as an automated code review tool and as an initial pass over submitted changes, I am fine with that, it sounds like it could save a lot of time. But to have it “enforced by default” and require us to annotate the source code just to prevent the formatter from messing it up is not something I am in favor of at all.

[PulkoMandy]

@waddlesplash

This is specifically what I would be against. Again, if it’s used as an automated code review tool and as an initial pass over submitted changes, I am fine with that, it sounds like it could save a lot of time. But to have it “enforced by default” and require us to annotate the source code just to prevent the formatter from messing it up is not something I am in favor of at all.

To me this is the same type of reasoning as “I don’t want to enable -Werror in the codebase in general because I prefer to write my code with warnings”. I don’t understand it. A pragma to disable a warning locally for a few lines of code? Sure, no problem. An exception for 3rd-party code? Yes, of course. But dramatically reducing the automation and the usefulness of the tooling for everyone else? I don’t think that’s great…

I hope you will be willing to spend a lot of time manually reviewing and fixing style issues from other developers then. Because that’s the price you have to pay for not wanting a mostly-automated solution.

But, anyways, I think I was clear that it was not about that. My question is: are you ok with adding clang-format disable markers around code that has custom formatting? Because otherwise, anyone else who touches the file and use a code formatting tool, will break your custom formatting. And I am not willing to put any effort in manually preserving it.

[nephele]

-Werror is a bad example, because it doesn‘t make sense in the first place.

Not ignoring warnings is nice, but forcing all warnings as errors locks you into a toolchain, effectively making sure the code only compiles with a certain compiler that only checks for warnings that do not appear in your code.

So gcc works, llvm will fail to compile if it gives more or different warnings. Not what I want, and exactly the same reason in this context why forcing clang format as the source of truth makes no sense.

[PulkoMandy]

It should compile without errors on all supported compilers. In general (if you support Linux, Windows, and a dozen compiler versions), this could be difficult. But Haiku is not in that case. Supported configurations are checked by the buildbots, so problems with -Werror are detected this way.

We already have -Werror enabled for a large part of th codebase and I will continue increasing it (hopefully with the help of others).

Non-fatal warnings are ignoreed by developers, wasting time debugging issues that the compiler had, in fact, detected.