Create a suitable coding style option set via clang-format

[ghost]

introduction

After discussing the coding style with the main upstream developer (which is btw. Kevin J. McCarthy)[1] [2], he said, he would welcome an coding style enforcement via clang-format (options: link).

Why is clang-format better than indent ?

• Gnu indent had the last stable release in 2008/2009, which makes it obsolete. So i don't think most bugs are very likely to be fixed.
• To cite the manual:

indent does not understand C. In some cases this leads to the inability to join lines. The result is that running a file through indent is irreversible, even if the used input file was the result of running indent with a given profile (‘.indent.pro’).

Clang-format does however understand C, because its part of the clang compiler.

new coding style must be really an improvement.

For instance,

$ clang-format -i -style=llvm  crypt-gpgme.c

produces, among others, the following diff:

-#define digitp(p)   (*(p) >= '0' && *(p) <= '9')
-#define hexdigitp(a) (digitp (a)                     \
-                      || (*(a) >= 'A' && *(a) <= 'F')  \
-                      || (*(a) >= 'a' && *(a) <= 'f'))
-#define xtoi_1(p)   (*(p) <= '9'? (*(p)- '0'): \
-                     *(p) <= 'F'? (*(p)-'A'+10):(*(p)-'a'+10))
-#define xtoi_2(p)   ((xtoi_1(p) * 16) + xtoi_1((p)+1))
+#define digitp(p) (*(p) >= '0' && *(p) <= '9')
+#define hexdigitp(a)                                                           \
+  (digitp(a) || (*(a) >= 'A' && *(a) <= 'F') || (*(a) >= 'a' && *(a) <= 'f'))
+#define xtoi_1(p)                                                              \
+  (*(p) <= '9' ? (*(p) - '0') : *(p) <= 'F' ? (*(p) - 'A' + 10)                \
+                                            : (*(p) - 'a' + 10))
+#define xtoi_2(p) ((xtoi_1(p) * 16) + xtoi_1((p) + 1))

I personally consider the version before significantly better. Therefore we have to make sure that our new clang-format option set really works in all cases of the mutt source code. If this is not possible, clang-format is not an improvement.

feel free to experiment and suggest new option sets!

This issue is meant to coordinate the experiments and suggestions in that regard. So i want to encourage you to post option-sets, of which you think they might be suitable.

regards,
toogley

[ghost]

Additionally, i want to point out that Kevin (the main upstream developer at the moment) doesn't like tabs - so i think we can make him happy if we suggest a spaces-only-based indentation. :) (which also solves the problem of having a mixed indentation)

[ghost]

I want to propose this clang-format config:

BasedOnStyle:  WebKit
BreakBeforeBraces: Allman
AlignTrailingComments: true
UseTab: Never
IndentCaseLabels: true

You just have to make a file called .clang-format with the above content and run

clang-format -i *.c

What do you guys think about that?

@kevin8t8 you're of course also encouraged to comment :)

[ThomasAdam]

I think so. In principle that looks OK. I'd also go as far as to suggest:

DerivePointerAlignment: false
PointerAlignment: Left

Which would put the pointer indicator with the variable name and not the type (after all, the code isn't written in C++).

Adding this into the travis-ci build to fail code which hasn't been formatted is easy enough to do as well, as-and-when.

[pickfire]

Actually is the upsteam using 2 spaces or 4 spaces? I found 2 spaces a bit confusing when there is lots of indentation.

[ghost]

@pickfire good point. upstream is using currently 2 spaces - which I dislike a lot. I'd love to change the default indentation to 4 spaces. @kevin8t8 what do you think specifically about this point?

@ThomasAdam Well, i don't have much experience using C pointers, so: what are the advantages of that?

Additionally: I liked the idea @flatcap made, which was to never leave out braces for if statements and such. However, i haven't found an option to add braces when they are missing. The description of the Attach value of the BreakBeforeBraces option sounded like it was the solution for that - but it didn't add braces around the return statement here:

if (something)
    return 0;

And i'd also like some sort of ColumnLimit but i don't know what a suitable value would be for that. 80, 100 or 120?

[ThomasAdam]

@toogley -- I think you're assuming that clang-format is somehow going to add additional things, when what you're wanting to use (with adding braces around single-line if-statements) is clang-tidy. See: http://clang.llvm.org/extra/clang-tidy/checks/readability-braces-around-statements.html

So if we're wanting to enforce a coding standard which has to add something (and not just rearrange what's there), we'll have to use clang-tidy as well: http://clang.llvm.org/extra/clang-tidy/index.html#using-clang-tidy

As for pointer alignment, I'm not sure what you're asking. There's a difference between:

char* foo

and:

char *foo

In the first example, C++ will typically align the pointer with the type. In the second, the pointer is aligned with the variable; that's typically more "C" and many C programmers will automatically code for that style, in my experience.

As for ColumnLimit -- 80 is the One True Value, surely? :P

[pickfire]

@ThomasAdam I normally uses char *foo because I thought that chat* foo isn't clear but I doesn't know that there is a difference between them.

[ThomasAdam]

@pickfire -- there is no functional difference, it's a question of style/cosmetics only, and I'm merely highlighting the difference between the two styles tends to lend itself towards a split between C++/C -- and that the char *foo case is more familiar to C programmers.

[ghost]

@ThomasAdam Ah, okay. Then i think that option makes sense. 👍

[darnir]

@pickfire: It's mostly a stylistic choice. But I think char *foo is far more clearer. Consider the following code:
char* foo, bar;. This would seem to imply that both foo and bar are char pointers, whereas in reality only foo is. The clearer definition is: char *foo, bar, which explicitly states that only foo is a pointer.

Char Limit should definitely be 80 on a C project.

[ghost]

Additionally, regarding the BreakBeforeBraces setting:
the value Allman is very mutt like - but i'm unsure how inconvenient that is for new developers who are accustomed to a value like Linux or Webkit.

=> Should we change that?

[ThomasAdam]

Have a look at: http://zed0.co.uk/clang-format-configurator/

Allman is fine for me.

[kevin8t8]

Thanks for Cc'ing me in this issue, @toogley. At this point, I can't answer definitively about style choices. I need to have a conversation with the other mutt developers. Any changes to the general style would need to be made by consensus with them, so it's probably best not to get too excited about big changes being made (at least in our repository).

For now, I'm more interested in how well clang-format works. e.g. does it do strange things that can't be turned off; can it closely match the current style; will it do more harm than good.

As I mentioned in the email referenced in the introduction, my time frame for this is 1.8 or maybe 1.9. I have a few piles of things to get through first, and can hopefully start paying attention to this in a month or two.

[guyzmo]

for the note: it'd be nice to also have a properly set editorconfig dotfile in the sources, that would help with having editors be correctly configured with the styles that will be decided and tested for the project 😉

[flatcap]

Here's my clang-format.txt

[flatcap]

On IRC, @toogley asked:

why did you specify a priority in there?

It was a bit of an experiment to see what the option could do.
If sorting the headers breaks the build then the header file is broken.

The header files in Mutt are a complete disaster area, but I haven't spent any time thinking about what to do about it.

[ghost]

@kevin8t8 i think it would be massively helpful, if we at least agreed on what indentation method we should use (tabs vs spaces).

[guiniol]

And also, how many spaces. I'd like at least 4, I think 2 is a bit too little.

[ghost]

@guiniol yeah, i agree with you. But moving from 2 spaces to 4 spaces is much more intrusive and we want to slowly increase the quality :)

[guiniol]

@toogley two spaces it is then!

[ghost]

preprocessor directives indentation

That means, at the moment:

#if HAVE_CONFIG_H
 #include "config.h"
#endif

is turned into

#if HAVE_CONFIG_H
#include "config.h"
#endif

this point is blocked by this LLVM bugreport on preprocessor directives indentation. According to the best and easiest way to achieve this would be to sent a pullrequest implementing this.

alignment of boolean operators and pointers.

and secondly the above described

#define hexdigitp(a) (digitp (a)                     \
                      || (*(a) >= 'A' && *(a) <= 'F')  \
                      || (*(a) >= 'a' && *(a) <= 'f'))
#define xtoi_1(p)   (*(p) <= '9'? (*(p)- '0'): \
                     *(p) <= 'F'? (*(p)-'A'+10):(*(p)-'a'+10))

still is turned in the more ugly

#define hexdigitp(a)                                                           \
  (digitp(a) || (*(a) >= 'A' && *(a) <= 'F') || (*(a) >= 'a' && *(a) <= 'f'))
#define xtoi_1(p)                                                              \
  (*(p) <= '9' ? (*(p) - '0') :                                                \
                 *(p) <= 'F' ? (*(p) - 'A' + 10) : (*(p) - 'a' + 10))

But i have trouble to estimate the possible fix of this bug.

current possible workaround

We can only resolve those issues in an rather ugly way by surrounding those things via // clang-format off and // clang-format on. I think using clang-format is still valuable because i haven't found any other problems and the resulting code looks really good IMHO

alternative: using the indent program.

But i haven't investigated that way.

[gahr]

@flatcap could we please have a rule that forbids casting void pointers and NULL (the only exception being varargs, of course ;) ?

[ghost]

@gahr Hm, does having the return type on its own line other advantages besides easier grepping? About the cons: I don't know, i haven't worked much with that style. But maybe that will lead to a (small?) hurdle for new contributors, which are most likely used to a style like:

int function foo(int x) {
   // ...

regarding disallowing casting void pointers and NULL: What are the problems when that is done? (Just curious here :) )

[gahr]

Uhm, if I had to come up with another advantage, I'd say that it is in line with both the GNU and the BSD coding styles.
So, there's a bit of a chance that new contributors having worked with either are not so unused to it ;)

Regarding casting void pointers, it's both unneeded - (void*) automatically converts to (T*) for any primary or composite T - and dangerous -(void**) doesn't unless you erroneously cast it.

[gahr]

Mozilla too.

[d-k-c]

Where-does-the-star-go moment:

• char *c; vs. char* c; (I think this one is mostly stelled in favor of char *c;)
• int* my_func(void) vs int *my_func(void)

[flatcap]

For functions, it would have go on the "type" line:

char *
function (void)
{
}

[gahr]

@d-k-c char *c;
@flatcap yes!

[xdgc]

I just learned about this conversation. I also just learned about clang-format. Sounds promising. My only solid input on the subject of reformatting until I get through the issue discussion is: for the sake of all that's holy, do not use Indent. Mainly because there are at least three incompatible versions of it.

I'm warm to clang, but I want to learn more about it. I also think that upstream we really need to standardize on something. The original standard is irritating to most people, and it's been unenforced for years. Upstream I'd be interested in using a hook to ensure style conformance.

[flatcap]

Welcome to the discussion, @xdgc

do not use Indent.

Don't worry, that wasn't really a contender.
It's important, though, to give the impression of choice :-)

I'm warm to clang, but I want to learn more about it.

clang-format is a very capable tool.
The config file above matches Mutt's "standard" fairly well

upstream we really need to standardize on something

YES. Upstream have been dithering for too long.

Unfortunately, Kevin keeps talking about a piecemeal approach -- fix the indent as new code is added. Mutt's development process is too slow for this to have any effect.

The original standard is irritating to most people

YES. We've had to compromise our styling to keep us close to upstream.
"2 space indent" is particularly unloved. "{}s on a new line" is another

clang-format can comfortably fix the indent and move the {}s.

Upstream I'd be interested in using a hook to ensure style conformance

This is where you have a problem...

However good a code-formatter is, there will always be cases where a human's arrangement of code is better for humans. clang-format will do all the heavy lifting, it will get you 99% of the way, but I don't think you'd want to automate it.

[flatcap]

Everyone is pretty much finished with this debate.
They've already moved onto talk about restructuring the code!

Tomorrow, I will close this thread and write up a guide for NeoMutt.

[guyzmo]

I think that using clang-format to modify the style is a nice way to enforce it throughout the code, but then, it'd be nice to find a style linter that will warn where the format is not correctly enforced, and have that part of the CI suite. But I don't know any good tool that could do the job ☹

[kevin8t8]

@flatcap wrote:

YES. Upstream have been dithering for too long.
Unfortunately, Kevin keeps talking about a piecemeal approach -- fix the indent as new code is added. > Mutt's development process is too slow for this to have any effect.

Huh? I think perhaps you are confusing me with someone else. I don't recall advocating piecemeal, and if I did I most certainly have not done so repeatedly.

You can see my comment in this very thread on Nov 7, 2016 and the link to my email in the introduction, where I mentioned this was on my list, possibly for 1.8 or 1.9. I do agree that upstream (me) is slow, but too long is just your opinion.

I'll repeat (again), that you are of course free to do what you want in your fork, including restructuring the code. That does not obligate me to march to your pace, or in the same direction as you.

[ghost]

@guyzmo clang-format can do that (basically applying it to a git diff and figure out if it has changed sth in the code), but the problem is mainly that we would have to put // clang-format off and such around code that should stay the way it is.

[ghost]

@kevin8t8 do you have already talked with the other mutt developers about this issue?

[xdgc]

Well, alrighty. Glad you guys worked this out for yourselves. :p

(Are you sure this isn't a fork? Your about pages say it's not, but I'm unconvinced. Looking at your commit log, and considering this major change, you're pretty close to being unable to merge anything from upstream without substantial edits. I appreciate your talking with us occasionally, @flatcap, but most of you guys have never spoken in mutt-dev about specific concerns warranting a divergent fork.)

Anyway: I am also a mutt upstream committer. It might have been me that you're thinking of, @flatcap — I don't think anyone has "[kept] talking about a piecemeal approach". I have said that we should follow style as we update the code, but that a total reformatting, while useful and desirable, needs to wait on a complete synthesis of outstanding patches. Once we have no patches that we care about, I think everyone upstream is completely fine with a reformat.

I'm interested in a hook. I'm pretty comfortable with using them. I don't intend to have anything auto-reformat code on commit, and am not sure what gave you that impression.

[ghost]

@xdgc We have internally discussed already whether we are a fork or not.: thread on our mailinglist

Our decision was that we will somewhen will be a fork - but currently aren't, as we upstream still fixes some important bugs.

[xdgc]

int
function foo(int x)
{

Obviously neomutt is making its own decision here, but the reason for this style is:

• not everyone uses an IDE, nor should they need to;
• placing the function name on a separate line greatly simplifies finding functions when not using an IDE
• placing { on a separate is conventional C style, and while I'm not totally opposed to the other approach and indeed use it in other languages, I don't see any reason to change C conventions because...
• ...seriously, you're worried about how many lines you can fit into a 1080p display?

Just a data point.

[xdgc]

That's my complaint, you've only discussed it internally. It has every attribute of a fork: I think it is already.

Let me be more clear on this: forking is not only making code incompatible. It is also the result of isolating your development community from the parent's.

[ghost]

@xdgc Well, first of all: i came relatively late to the party so therefore i don't have the frustration of the other neomutt members. But as far as i understood, neomutt members had multiple times proposed refactoring patches which were rejected by upstream.

That means: my impression of the consens among the neomutt developers is that we, on the one hand, would like to work more closely together with upstream mutt. But on the other hand we have the desire to make the mutt code better and refactor things.

Another thing that complicates our relationship is that, honestly some of your decisions aren't always understandable: As far as i know upstream once removed a comment block for a function we had written - which had explained what the function does and such.

I have honestly difficulties to understand that. The only reason i could think of is, that the comment was useless. But i doubt that, because neomutt's work to make mutt's code better understandable was in my opinion in the last months really great.

[kevin8t8]

@toogley Really, you're going to spout hearsay about what supposedly happened? I'll tell you my side of the story then.

Flatcap, completely unknown to the community, introduced himself to upstream by proposing to reformat all the code. I had finally gotten the development going again and released 1.5.24, and was starting to look at outstanding patches. The timing was wrong, and in any case no one was inclined to give a complete stranger such access. So he was told no and it was suggested he start small and build up to larger changes. Instead, he disappeared and 3 months later announced NeoMutt.

Then, about a week before the 1.6.0 release (the first stable release in 10 years) and inside the stabilization code freeze, he decided it would be a great time to run some code scanner and file a bunch of tickets with patches fixing every warning he could find. At least some of the patches caused bugs, and the timing was ridiculous.

So this is the kind of "help" your fearless leader offered, and how he ingratiated himself to our community. There are multiple sides to every story. I'm sure he'll tell you how hard he tried to get anything accepted, but from our side he exhibited extremely poor social skills and judgement.

I've worked hard on getting some patches in, and have started with a couple from NeoMutt: the sidebar, and the compressed folders being the most notable. Both of them were reformatted very prettily but had substantial underlying problems with them. I worked my butt off to fix the issues and commit them, and if a comment got deleted in the process, well cry me a river. I have no memory of what you are talking about, but in any case, it's my/our prerogative what gets committed upstream.

If people are really interested in working with upstream, it's quite simple: subscribe to mutt-dev, and start small. Let us get to know you with bug fixes and small patches, and then work up to bigger changes. No, not everything will be accepted, especially mega-refactors and redesigns before you've demonstrated an understanding of the code. But I've been trying my best to review and accept patches, and it's much better than it was a couple years ago.

Forking is a hostile act, and the faux-outrage combined with the denial "we aren't a fork" to justify it are disgusting.

[xdgc]

I would love for neomutt developers to work more closely with upstream. We're not unreasonable. We do insist on not breaking things, to the extent we can predict it, and on maintaining support for all the things we support; as a result our review process can be difficult. The sidebar patch that gets a lot of praise is a good example: we opposed it for many years not because it's a bad idea, but because the code was undercooked. Once someone took care of it and did it right, we merged it.

I've been involved with mutt for 18 years. I've written a lot of new features. Getting things committed is primarily a matter of having a good idea and coding it well. That can take time and patience.

IMO the biggest and most real concern with the upstream community is that we're slow. We have limited time at our disposal, but we know this and would very much like the additional help. But with a few exceptions, I've never even seen most of your names. Richard had one encounter with us in 2015, concerning a complete reformat of the code — a big deal! — before forking in 2016. It just... from our POV it doesn't even feel like anyone here really tried to engage. :(

I can't speak to the comment block that you heard we removed, obviously. We don't do these things maliciously. We want the contributions and the help.

[xdgc]

Kevin's leadership has been a huge improvement. The rest of the committers have been with mutt longer, but he's taken time that we've been unable to provide and, IMO, very much turned the project around.

It's hard to express how saddening it is that at the same time we're making that progress, we see two forks (also mutt-kz, whose maintainer has literally never posted to a mutt ML) splitting off with no intention of future closure and with cooperation as an afterthought. "Heartbreaking" is perhaps a stretch, but it certainly comes to mind.

[ghost]

@kevin8t8 and @xdgc thanks for your point of view.

Forking is a hostile act, and the faux-outrage combined with the denial "we aren't a fork" to justify it are disgusting.

Okay, i think you got that wrong somewhere. At least my impression is that neomutt is not hostile. The reason we don't call neomutt a fork at the moment is that we are in a
kind of "abeyance" phase. That means, we have to figure out if a fork is a reasonable idea, have to specify how to best "execute the fork". And, especially we aren't even sure about when we want to do that. Furthermore, some of our developers feel that they don't understand the code well enough. Therefore, our conclusion is that we aren't a fork for now.

The above style changes are at the moment just ideas being discussed. Our coding style matches your's at least close IMHO (note: of course, our consensus how a "better" coding style should look like is also written there, but for now we haven't implemented those.)

No, not everything will be accepted, especially mega-refactors and redesigns before you've demonstrated an understanding of the code. But I've been trying my best to review and accept patches, and it's much better than it was a couple years ago.

Do i understand you correctly that you don't want to review these big refactorings, because you do consider it risky to invest time in reviewing a patch which is unsure if its well written?

It's hard to express how saddening it is that at the same time we're making that progress, we see two forks (also mutt-kz, whose maintainer has literally never posted to a mutt ML)

I completely agree with that.

But with a few exceptions, I've never even seen most of your names. Richard had one encounter with us in 2015, concerning a complete reformat of the code — a big deal! —

Hm, but honestly, reviewing style changes shouldn't be that much work, compared to reviewing patches? I understand why you're cautious about big changes. But also i think that mutt's code needs some big changes... (like for example the formatting, to remove all tabs and such).

I understand also that @kevin8t8 said that's on his todo list - but i don't understand why you just haven't merged @flatcap 's changes in - as you only have to review and not do the work yourself.

I would love for neomutt developers to work more closely with upstream.

Great! :)

Additionally: @kevin8t8 i don't want to make accusations on anything. I just want to understand you. :)

I've worked hard on getting some patches in, and have started with a couple from NeoMutt: the sidebar, and the compressed folders being the most notable. Both of them were reformatted very prettily but had substantial underlying problems with them.

What do you mean by that?

[flatcap]

I've spent a year reaching out to people, bringing them together, encouraging them to contribute.
I've successfully built a community. When Mutt can offer that, then there'll be no need for NeoMutt.

[flatcap]

CLOSED

As the on-topic part of this conversation is concluded, this issue is closed.