Style guide and clang-format.

A while back I asked about the possibility of bringing our style guide closer to LLVM’s. To that end, I made a clang-format configuration file for LLDB and included it at the base of LLDB’s repo. At the time I didn’t know this, and I had my own hacked up solution, but it turns out it’s very easy to use clang-format to format only those lines in a CL that have changed. Here’s how you do it. Steps 1-3 only need to happen once, and after that you only need 1 command to invoke clang-format.

  1. Build clang-format. This is very easy to do, since everyone building LLDB already has clang checked out. So all you need to do is point CMake to the root of your llvm source tree like this:
    src/lldb/llvm> mkdir build
    src/lldb/llvm> cd build
    src/lldb/llvm/build> cmake -G Ninja …
    (wait a while)

Then build clang-format
src/lldb/llvm/build> ninja clang-format

  1. Copy clang-format and the git clang-format command extension to somewhere in your path
    src/lldb/llvm/build> mkdir ~/git-clang-format
    src/lldb/llvm/build> cp bin/clang-format ~/git-clang-format
    src/lldb/llvm/build> cp …/tools/clang/tools/clang-format/git-clang-format ~/git-clang-format
    src/lldb/llvm/build> export PATH=$PATH:~/git-clang-format

  2. Make sure that git-clang-format is executable
    src/lldb/llvm/build> chmod +x ~/git-clang-format/git-clang-format

  3. Run git clang-format!
    src/lldb/llvm/build> cd …/…
    src/lldb> git clang-format

This will run clang-format against the files you have staged. It will only reformat lines which are part of the diff, so you don’t have to worry about a lot of pollution.

It would be great if more people started using this. It makes it so you don’t have to think about formatting as you type. There are a very few cases where it gets things wrong, so be careful. lldb-forward.h is a good example of this. When you have types that are aligned or justified it will remove the alignment. That’s about the only limitation though.

There is 1 section I can think of where the clang-format specification file differs from LLDB’s documented coding convention (BTW, thanks Jim for getting this up). I would like to propose removing this line from LLDB’s style guide. It is this part:

Another place where lldb and llvm differ is in whether to put a space between a function name, and the parenthesis that begins its argument list. In lldb, we insert a space between the name and the parenthesis, except for functions that take no parameters, or when the function is in a chain of functions calls. However, this rule has been applied rather haphazardly in lldb at present.

The clang-format file simply does not put a space there. I think it is worth removing this line from the style guide for a couple reasons:

  1. It differs from LLVM (actually I want to remove even more lines from the style guide for this reason, but for now I only want to ruffle a few feathers, not all the feathers :slight_smile:
  2. clang-format does not have a way to specify rules with exceptions that are this detailed.
  3. It is not consistently applied in the codebase as is.
  4. The rule is confusing, and the benefit isn’t clear enough to warrant having its inclusion, especially given that the confusing nature of the rule is probably a leading cause of #3.

To be clear: I’m not saying we ban this. I’m saying remove it from the style guide. If you want to still do it, then by all means still do it. And if you use clang-format, clang-format will remove the space for you.

Thoughts? Suggestions?

one thing I forgot to add. The reason I’m bringing this up now is because I’ve seen more activity in LLDB over the past 2 months or so. Off the top of my head, we have people actively supporting Linux, Android, FreeBSD, Apple, Hexagon, multiple platforms with lldb-mi. I’ve also started to see more participation from people who started out on the LLVM side of things.

So I think it’s worth reconsidering the benefit of a specific set of rules vs. how “welcoming” our code is to people from the LLVM side of things. One area where LLDB currently is lacking is with regards to LLVM expertise. Most people are focused on details of a specific platform’s debugging experience, and I think the benefit to LLDB would be very large if we had more people from the LLVM side hacking on things.

A while back I asked about the possibility of bringing our style guide closer to LLVM's. To that end, I made a clang-format configuration file for LLDB and included it at the base of LLDB's repo. At the time I didn't know this, and I had my own hacked up solution, but it turns out it's very easy to use clang-format to format only those lines in a CL that have changed. Here's how you do it. Steps 1-3 only need to happen once, and after that you only need 1 command to invoke clang-format.

1) Build clang-format. This is very easy to do, since everyone building LLDB already has clang checked out. So all you need to do is point CMake to the root of your llvm source tree like this:
src/lldb/llvm> mkdir build
src/lldb/llvm> cd build
src/lldb/llvm/build> cmake -G Ninja ..
(wait a while)

Then build clang-format
src/lldb/llvm/build> ninja clang-format

2) Copy clang-format and the git clang-format command extension to somewhere in your path
src/lldb/llvm/build> mkdir ~/git-clang-format
src/lldb/llvm/build> cp bin/clang-format ~/git-clang-format
src/lldb/llvm/build> cp ../tools/clang/tools/clang-format/git-clang-format ~/git-clang-format
src/lldb/llvm/build> export PATH=$PATH:~/git-clang-format

3) Make sure that git-clang-format is executable
src/lldb/llvm/build> chmod +x ~/git-clang-format/git-clang-format

4) Run git clang-format!
src/lldb/llvm/build> cd ../..
src/lldb> git clang-format

This will run clang-format against the files you have staged. It will only reformat lines which are part of the diff, so you don't have to worry about a lot of pollution.

It would be great if more people started using this. It makes it so you don't have to think about formatting as you type. There are a very few cases where it gets things wrong, so be careful. lldb-forward.h is a good example of this. When you have types that are aligned or justified it will remove the alignment. That's about the only limitation though.

There is 1 section I can think of where the clang-format specification file differs from LLDB's documented coding convention (BTW, thanks Jim for getting this up). I would like to propose removing this line from LLDB's style guide. It is this part:

Another place where lldb and llvm differ is in whether to put a space between a function name, and the parenthesis that begins its argument list. In lldb, we insert a space between the name and the parenthesis, except for functions that take no parameters, or when the function is in a chain of functions calls. However, this rule has been applied rather haphazardly in lldb at present.

The clang-format file simply does not put a space there. I think it is worth removing this line from the style guide for a couple reasons:

1) It differs from LLVM (actually I want to remove even more lines from the style guide for this reason, but for now I only want to ruffle a few feathers, not all the feathers :slight_smile:
2) clang-format does not have a way to specify rules with exceptions that are this detailed.
3) It is not consistently applied in the codebase as is.
4) The rule is confusing, and the benefit isn't clear enough to warrant having its inclusion, especially given that the confusing nature of the rule is probably a leading cause of #3.

I actually do think that having the space between the complex visual noise of the argument list and the function name makes it easier to detect functions when scanning code, which was why we did it this way to start. That and from years of working on FSF code, Jason & I were used to it. It was a hard-and-fast rule for FSF code, but then for C++ this sort of thing:

target.GetProcess ().GetSelectedThread ().GetStackFrameAtIndex (0)

just looks stupid. So we don't use it there. I have a Quixotic desire to have some not hard-and-fast rules in the coding conventions, and rather to decide based on what looks good, because "I'm a free man, dammit!".

But in reality, I don't much care one way or the other about this one.

Jim

That’s fine, and that’s why I’m proposing just removing it from the style guide. Because leaving it in, and saying “We do this, but actually… not everyone does, but you can do it if you want!” is kind of redundant and unnecessary. Unfortunately clang-format, being a tool, does have hard and fast rules. So it seems like we can just say “do it if you want, but clang-format will remove it for you, and hopefully you use clang-format (because it has many other benefits as well)”

That's a little problematic, because it's easier if the lldb style guide falls back to the llvm style guide on all the issues it doesn't specify. So if we leave the line out, then that should mean use the llvm rule.

Jim

I was trying to give you the benefit of the doubt :slight_smile: TBH I’d be even happier if we just use the LLVM rule consistently. But it’s often easier for people to do this slowly. From your original response though it sounds like you might be fine just removing this rule and going with the LLVM style. If so, I’m definitely all for that.

If it makes you feel any better, I’m not crazy about some of their rules either. And actually they aren’t even crazy about some of their rules. But it does help with consistency and readability for people who work across all of the codebases, and I think that kind of cross-project pollination is really helpful for the individual projects, as well as the community as a whole.

I was trying to give you the benefit of the doubt :slight_smile: TBH I’d be even happier if we just use the LLVM rule consistently. But it’s often easier for people to do this slowly. From your original response though it sounds like you might be fine just removing this rule and going with the LLVM style. If so, I’m definitely all for that.

This is completely consistent with what I’ve advocated for the team here: we deviate from the established LLVM convention for new code only in ways that are well-documented, consistently followed in the established code base, and for which we have a compelling rationale. I think sticking with LLVM style here makes the most sense.

Of course there will always be the temptation to make sure that minimal additions to existing files don’t stand out as egregiously different. Hopefully some common sense can be applied in these cases.

Kate Stone k8stone@apple.com
 Xcode Runtime Analysis Tools

Great to hear! I will make this change to the style guide in the coming days if nobody objects further.

In the meantime, I would love for you guys (and anyone else for that matter) to try out clang-format. It would be great to work out any kinks in the existing rules file. For cases where there’s a compelling enough reason, if clang-format doesn’t support what we need, we can get it added. But for most things I think it should already either be correct in the existing rules file, or be fixable with a few tweaks and no modifications to clang-format itself.