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.
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…
well, yes, “improved clang-format” in this context is haiku-format, since some things may not be upstreamable.
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.
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.
5 Likes
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.
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.
2 Likes
-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.
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.
1 Like
I think my position is somewhat in the middle. From my point of view:
- The output of haiku-format must be in compliance with the code guidelines.
- On certain areas, the guideline can give a contributor a choice to deviate from those guidelines for (subjective) readability.
There is a reason the direction of the bot was to make suggestions in code review, rather than just giving a thumbs up or thumbs down. These are now ‘resolved’ suggestions, if the bot is more reliable (i.e. when we ever reach goal #1), they could be unresolved by default and a contributor can decide to apply the suggestion, or to override the suggestion with a comment (or with an opt-out in the source itself).
But for this to become feasible, I am certain that we must get to a point where the output of haiku-format is compatible with the formatting part of our code style. This way there is a compromise position and we can have the discussion about adjusting the code style without the threat of ‘obligatory code formatter’ hanging over us.
Having said that, I still think that adjusting declaration style to something compatible with haiku-format is essential and it should be part of the obligatory coding guidelines as I think they will affect a large percentage of patches, and thus a large number of false positives. It is also an aspect of our source code where it does not make sense for a single line (or a few lines) to deviate. Having a normalized class definition with a few table-style members would be odd. If we are talking about builders, or a particular way of breaking up chained operators over multiple lines/additional whitespace, then these only affect their own lines, and not the ones above and below it.
1 Like
Or you just format only the lines touched by your changes: Clang Format script for patch reformatting
I think haiku-format should not transform compliant to uncompliant code.
Emitting any compliant code does not make sense to me as a goal.
If haiku-format encounters stuff it does not know how to format, for example the class headers, It should just not change them at all.
The all or nothing aproach of such formatting tools makes no sense to me, you are preventing using multiple tools that solve different problems after one another because haiku-format in such a chain would just undo fixes of other tools.
Making haiku-format more compliant with our guidelines, and changing them slightly for clarification, is a good goal I think.
That sounds like defining that llvm is just unsupported, because it could emit different warnings.
Porting to clang should be fun if you want to enforce that compiler flag .-.
It will force contributors to touch pretty much every part of the tree that gets new warnings instead of fixing only the relevant potential compilation bugs.
I expressed a similar view before. The following example borrowed from Class declaration - cppreference.com illustrates the downside of keeping the current table alignment style:
struct S
{
template<class CharT>
struct NestedS
{
std::string s;
static volatile unsigned long long i;
virtual void f0(int) = 0;
[[nodiscard]] bool bar();
template<typename T> void g(T&& n);
} d5, *d6;
// Column 100 --
// |
// V
template<class CharT> NestedS(std::basic_string<CharT>)
-> NestedS<CharT>;
S(std::size_t R, std::size_t C)
: d0(C), d1(R * C) {}
explicit S(int) {} // How to align this?
~S();
static volatile unsigned long long d0;
int d1;
int a[10] = {1, 2};
static const int d2 = 1;
virtual void f1(int) = 0;
std::string d3;
std::string* d4;
std::string f2(int);
[[nodiscard]] bool foo();
template<typename T> void f(T&& n);
using namespace clang;
enum class [[enum_extensibility(open)]] Direction : unsigned char { NORTH, SOUTH, EAST, WEST };
// How is the above supposed to be aligned?
// Align or not align?
typedef NestedS value_type, *pointer_type;
};
Not wanting to mandate a ‘strict’ tool is a fair position. However, I do not know if the alternative you propose is feasible. The existing tools all make trade-offs how they work internally and as such what configuration they can offer, and what features they actually offer.
In my investigation, the tooling is on a spectrum. There are some very relaxed tools up to very strict tools. On the relaxed side I think you have uncrustify, which is relaxed because you can configure it to ignore a lot of things, on the very strict side you have tools like rustfmt for Rust and Python black, which emit consistent output (i.e. if the compiler/interpreter parses two input files to the same AST, the reformatted version will be identical).
I think clang-format is to the right of the middle on that spectrum, where it has some flexibility to be configured to let some variation in output for the same AST exist, but it definitely is not configurable for very complex context-sensitive logic, nor is it configurable to just let variations of the rules stand under certain conditions.
And this is where the economic side of the argument comes from: we are in the operating system business, not in the code formatting business. It is a choice whether we want to spend our resources on adapting a code formatter to our style, rather than just choosing to adjust our style to get more value out of the tools. Just like it is a trade-off to spend a lot of our intellectual time nagging people about style violations, rather than just focusing on the content of their contributions (and letting the tools nag about style violations).
My position is that it is worth adjusting our style guide to the reality of the tools, in order to get the value for them. How and where we want to adjust is an ongoing community conversation and clang-format/haiku-format may not be the final word. However, at this stage I am quite confident in the claim that our declaration style currently is not supported by the tools, so we would have to look at that one first. That was the intent of this proposal.
3 Likes
This example looks engineered to be in the worst case.
Anyway, here’s an attempt. I changed:
- Reduced the level of indentation in general, there was no reason to have so much spacing
- Grouped the static, virtual, nodiscard, and templates in one column. The “volatile” is up for debate, but, since it makes no sense to return a volatile int, I think we can ignore this case (or maybe talk about pointers-to-volatile). Anyways, I think it could be moved to the “return type” column without too much problems here, especially if not using “unsigned long long” which is just a verbose way of saying “uint64”. Included an example that way as well.
- Return types with the → notation are moved to the next line and indented one tab. This aligns them with return types specified in the traditional style, makingthem easy to find.
- Group functions together and variables together. Group static variables together. Each such defined “block” does not need to be aligned with all the other blocks, although in this case, it works fine while preserving the alignment.
struct S
{
template<class CharT>
struct NestedS
{
public:
std::string s;
static volatile unsigned long long i;
static volatile uint64 j;
public:
virtual void f0(int) = 0;
[[nodiscard]] bool bar();
template<typename T> void g(T&& n);
} d5, *d6;
public:
template<class CharT> NestedS(std::basic_string<CharT>)
-> NestedS<CharT>;
// Column 120 --
// |
// V
S(std::size_t R, std::size_t C)
: d0(C), d1(R * C) {}
explicit S(int) {} // How to align this?
~S();
virtual void f1(int) = 0;
std::string f2(int);
[[nodiscard]] bool foo();
template<typename T> void f(T&& n);
public:
static volatile unsigned long long d0;
int d1;
int a[10] = {1, 2};
static const int d2 = 1;
std::string d3;
std::string* d4;
// No specific alignment for enum and namespace definitions.
using namespace clang;
enum class [[enum_extensibility(open)]] Direction : unsigned char {
NORTH,
SOUTH,
EAST,
WEST
};
// Also no alignment for typedefs, as far as I know
typedef NestedS value_type, *pointer_type;
};
That’s not too bad, I think? Also, totally hypothetical since we don’t use much of these features in Haiku. Sure, in theory there could be a static nodiscard template. Then it don’t fit and we break the alignment or add extra linebreaks. I think that’s fine?
Yes, that’s a hard requirement I think we can all agree on (and discuss how to reach it, since it is currently not the case: we either need to change the guidelines, or change the tool).
I also think that’s acceptable. But we can also discuss the “how”. There seem to be two opinions here. One is that the tool should not touch code that already matches the guidelines (after being formatted manually). Another is that the tool could do that, but areas where the manual formatting is really necessary should be marked with “clang-format disable” markers in the sourcecode.
I’m not sure how the first option is going to work exactly, at least not by using clang-format as a base for this work. Sure, we can limit the reformatting to lines modified in a given patch. But, some of these lines could be previously manually formatted. What to do then? I think that reduces the usefulness of the tool a lot.
I work with automated and systematic clang-format in my paid job, and I never had an issue with the way it formatted the code (unlike other tools I used in the past, which sometimes made strange decisions). Or, when I was not happy, it usually was a setting that I could change. So I find it strange that there is such strong opinions in support of manual formatting here. I would like to see examples where this really makes a difference, and how many of these there are. For me, it is great that I can make my changes, run a formatting tool, and not have to check and fix the formatting manually ever. And, yes, as I have already said, I think this saves so much time and effort that I would be ready to give up on the “aligned” formatting style if it leads to such an automated workflow. But not if I get “worst of both world”: still needing to manually format code or preserve existing formatting, and still needing to tell other people to do so in code reviews, making them give up on getting their code submitted after 10 different people have given 10 different opinions on how the code should be formatted. In that case, I don’t see what the gain is. I will probably go for it anyways, and continue pushing for more automation of code formatting to be put in place later.
Haiku-format knows how to format everything. It just doesn’t agree with the current definitions in our coding guidelines.
I don’t understand what you mean. The one goal of haiku-format is to format the code. That’s all it does, and it should do it fully and completely. There should be no need to combine it with other tools that do the same thing. Let’s just make it work right? Or if we can’t or don’t want to, we have the option to mark the parts of the code it shouldn’t touch. This could be explicit clang-format disable rules, or, maybe, it could be some special casing built into the tool (either as clang-format patches or as a wrapper script for it) to tell it which parts of the code should be ignored. But, honestly, I don’t understand why we would go through so much effort instead of changing our coding style a bit, to something that not only clang-format can understand, but also developers can actually understand because it is more clearly defined. The current rules are not in that case, and while it creates some freedom in formatting for people who have been contributing for a long time and somehow know what “looks right”, it is a big problem for newer contributors who have to apply feedback that does not seem to correspond to any written rules. This is just rude to new contributors, to them it sounds like we are making up rules as we read their code just to force them to put more effort than needed. And “new” here is a broad term. You can find in this very discussion that both apl-haiku and x512, who have been contributing for a while, are complaining about this. And I can see it in the code reviews I do, as well.
5 Likes
And now apply Haiku coding guidelines rule that identifiers should be descriptive and not abbreviated. So you easily go out of 100 characters limit after method name.
1 Like
We have been using this style for more than 20 years now, and it used to be a 80 column limit. Without that much problems. And, again, if you reach the line length limit, you add a linebreak. No big deal.
Anyway, it doesn’t matter. As I have already said, if a non-aligned style allows us to move to more automated formatting, I support that. Not because it is better in terms of readability (as we have clearly seen in this discussion, this is clearly a personal opinion anyways), but because it is easier to automate. That saves me a lot of work, so it is good.
5 Likes
^ This is my opinion too. Accepting the “out of the box” style should make for easier on-boarding and should mean the project has more productive hours available from existing contributors.
6 Likes