What're the thoughts on Google C++ Style Guide?

I don't have strong personal opinions on the style guide we use just as long as we can settle on something fast and future contributors wouldn't find it a problem, so I'm looking at pre-existing ones that carry some weight that we could adopt

I can't find anything good to say about the Google C++ Style Guide. There are two somewhat dated books but they get the fundamentals right:

https://www.amazon.com/Exceptional-Style-Engineering-Programming-Solutions/dp/0201760428/ref=asap_bc?ie=UTF8
https://www.amazon.com/Coding-Standards-Rules-Guidelines-Practices/dp/0321113586/ref=asap_bc?ie=UTF8

Granted, I think they only have 2-3 pages on things like what clang-format does, because the authors just say that you should be consistent and move on.

The clang-format settings that we have are my attempt at matching what the code currently does, though I that's not my personal preference. I'd prefer not to step on other people's preferences just because I have different ones. :)

In any case, this is just a starting point. I'd like to start running clang-format and making PR's with various changes and see what people think. The code is currently inconsistent, which is probably the worse of all worlds.

OK, that's fine, it's out of the window then.

👎 I can't make use of clang-format 4.0...

Yeah it's a bit problematic for me as well:

vadi@volga:~$ sudo apt install clang-format-4.0 
Reading package lists... Done
Building dependency tree       
Reading state information... Done
Some packages could not be installed. This may mean that you have
requested an impossible situation or if you are using the unstable
distribution that some required packages have not yet been created
or been moved out of Incoming.
The following information may help to resolve the situation:

The following packages have unmet dependencies:
 clang-format-4.0 : Depends: libllvm4.0 but it is not going to be installed
E: Unable to correct problems, you have held broken packages.
vadi@volga:~$ sudo apt install libllvm4.0
Reading package lists... Done
Building dependency tree       
Reading state information... Done
Some packages could not be installed. This may mean that you have
requested an impossible situation or if you are using the unstable
distribution that some required packages have not yet been created
or been moved out of Incoming.
The following information may help to resolve the situation:

The following packages have unmet dependencies:
 libllvm4.0 : Depends: libstdc++6 (>= 6) but 5.4.0-6ubuntu1~16.04.4 is to be installed
E: Unable to correct problems, you have held broken packages.
vadi@volga:~$ 

Shouldn't be a blocker though, the tool is out and available - I'll see what I can do.

@SlySven check out http://llvm.org/releases/download.html (when it's up).

It should work with previous versions, with the exception of 1-2 fields perhaps, which have changed from being boolean to being an enumeration.

I just downloaded it earlier from that link, so I'm somewhat surprised it went down right after. What version do you both have access to?

I see that that download site has a Debian 8 (Jessie) clang download as a pre-built binary - but I realise I am using "Jessie-backports" so I can get 3.8 versions if I explictly ask for them with clang-3.8/clang-tidy-3.8and clang-format-3.8! 😄

Same for me, 3.8 is what I've got installed OK.

@ahmedcharles mentioned there are static 4.0 binaries available - but if it's just a few fields and nothing major, let's stay on 3.8?

Yeah, we want to use tools that other will also be able to use to format their contributions.

So that means putting a hold on this PR?

Sure. Can you just try running clang-format -i src/*.h src/*.cpp and seeing if it complains at all?

The only error message it spams is YAML:111:21: error: unknown key 'IncludeIsMainRegex'.

Um, I'd rather not do an in-place edit (-i option) just to test it...!

That said, don't recommend running it over all files - I think one of our huge files is causing it to require a lot of memory to deal with. Need to figure out which and see what can we do about it.

You can remove the -i option and it will just print to stdout.

As for the memory usage, that's abnormal, since I just ran the exact same commands. You can just remove the 'IncludeIsMainRegex' from the file and see if that helps?

It was trying to format src/qrc_mudlet_alpha.cpp which I can understand being a problem. So in practical scenarios the memory usage will not be an issue.

@SlySven how'd you get default fields in constructor to align? Did you use uncrustify?

@SlySven To be honest now that we'll have a formatting tool in place, I don't think we have to require braces around single-line blocks, because the problem solves itself: no braces is only an issue if you don't get your indenting right, but now we can, very easily.

That's not entirely true, because it's unlikely that clang-format will end up being used to format the entire code-base, unconditionally. There's some code that will just look better manually formatted.

Besides, characters that can be seen {} are always better delimiters than characters that can't be seen, .

With the fact that we will now get lines like } else { means I'd like to see braces being used everywhere (at least until I get used to the change - and those visible delimiters do help!) I recently sent in a small patch to the Input team for the Linux Kernel and their "style" allows single line un-braced if/else branches only if BOTH branches are 1 liners - but that is not sommat I'd normally want to do myself.

The comment that says to revisit it is there because it won't be 200 forever. It's just a starting point. I'm not even changing that in this commit, so why would that block updating everything else?

The expected use case for clang-format is making a commit and then running:

git clang-format HEAD~ # formats the new commit
git add -p # add/modify formatting changes, as needed (if there's an unwanted one, just don't add it)
git commit --amend # amend the commit with the changes above
git reset --hard # remove the unwanted changes from the working directory, if there are any

This means that for the fairly short time I'm actively making formatting changes, you would never have to actually scroll your editor, because it would never edit code while you were editing it.

Though my primary point is: PR's can't be expected to fix every bug and the existence of a bug in the file doesn't warrant preventing other changes to the file.

I still don't get the proposed workflow however - why not get the formatting done right once, so we can apply it and continue with the new way once and for all, instead of iterating on it and littering git blame history with continuious reformatting?

I would like us to have an agreed formatting setup that we can just hammer out/configure once and then can be used any-time from the QtCreator beautify menu. I am prepared to accept that some of my styling wishes, like the hanging indent I use on "[ TYPE ] - message to be shown on main console" - so that:

• the source code layout approximates the on-screen layout
• I can get the English lines approximately 80 chars long

won't survive being tidied.

Is it worth commenting in the .clang-format file those settings that do not work before 4.0 or have different behaviour between 3.8 and 4.0

I have just found that QtCreator's handling of the clang-format tool allows some predefined selections besides the "File" option to use the dot file in the current or ancestor directory. In addition there is an internal system to name your own style and then you can (copy and) paste in your own recipe and it seems to recognise most settings and attempts to document the possible options, but not much else:

I'm not sure about the confusion, given that I gave the exact commands above. I know that workflow works and due to the fact that it uses git commit --amend, it doesn't result in commits which have only formatting changes.

I'm a git power user, so I prefer using rebase/amend/squash/etc to make commits look like what I want, rather than just making commit after commit which will mess up the history of the project.

As far as I can tell, the current concerns are all related to workflow and potential lack of knowledge rather than strictly being opposed to what I'm proposing, at least in this commit. Hit me up on gitter if you want to talk about it.

The exact confusion is more about how the process will work - I'm lost between PRs that are formatting entire files (good) and comments saying that this is just a starting point for the settings (confusing - if this is not the final version, it implies we'll be reformatting them again). Sorry for being difficult on this.

Oh, ok. The changes I submit are from me running clang-format and then I run git add -p and pick the changes I want and occasionally edit them. I prioritize making changes that won't need to change in the future at all and I don't pick changes that will need to change. At some point, the set of changes will be small enough that I can tell whether I need to tweak the settings to make them better or whether I should just accept that change or whether we should just leave it formatted as it is and never really run clang-format on it in the future.

Basically, I'm looking at and deciding on every change whether it should go or not, that's how I'm making sure that this process converges.

Okay, that makes sense. What can we tell new contributors to do in the future then to make sure their changes are formatted right? I suppose ask them to selectively format the parts of the files they've changed instead of running the formatter on the entire file? The former would be a lot more simple, but it would break custom formatting we have.

My perfect state on this is that we have all the rules encoded within clang-format so it's the canonical source of truth on this and we can just press a button and it'll spit out a result we'll be happy with, seems the easiest way to do these things.

The most flexible workflow that I know of is this:

git clang-format HEAD~ # formats the new commit
git add -p # add/modify formatting changes, as needed (if there's an unwanted one, just don't add it)
git commit --amend # amend the commit with the changes above
git reset --hard # remove the unwanted changes from the working directory, if there are any

clang-format's editor integration normally supports formatting selections and formatting individual lines, so that shouldn't be a huge issue.

Unfortunately, C++ is really complicated in terms of syntax and I don't really know if we have lots of common places where we'll want to not format things the way clang-format does. There are a few cases of #defines that I prefer being indented that clang-format removes the indent. I can't know if there are other cases which are hard to handle without finishing reviewing the remaining 60k or so lines of changes. :)

The mental model for formatting Go, for example, is like you suggest. You just press the button and the outcome is fine. But Go was designed with that in mind, C++ wasn't and C++ has some weird corners. I'm just taking the conservative position that 100% automatic formatting may not be optimal and we may want to avoid certain sections. clang-format was designed with this in mind and therefore, makes it easy to achieve that result.

Alright, fair enough, we'll live with it and just ask contributors to selectively format stuff - plus the git commands for the power users.