Qmk_firmware: What should the setting of IndentPPDirectives be?

[skullydazed]

Pre-processor formatting

Currently our preprocessor directives don't follow a consistent style. Sometimes the indent level matches the code's indent level, sometimes no intenting is used at all, and sometimes PPD's are indented to their own indent level.

Since we are switching to clang-format we have to choose one of the 2 pre-defined options, neither of which are a good fit for all the ways PPD's are used in QMK. We are looking for feedback from people who work on core code to make this decision.

Possible PPDIS values:

None

PPDIS_None (in configuration: None) Does not indent any directives. You can see this in the future_ppdis_none branch.

#if FOO
#if BAR
#include <foo>
#endif
#endif

After Hash

PPDIS_AfterHash (in configuration: AfterHash) Indents directives after the hash. You can see this in the future branch.

#if FOO
#  if BAR
#    include <foo>
#  endif
#endif

Before Hash (Later, after clang-format 9.0.0 propagates)

PPDIS_BeforeHash (in configuration: BeforeHash) Indents directives before the hash. This option is only available in clang-format 9.0.0 and later. The latest version shipped by homebrew (and perhaps other systems) is 8.0.0.

#if FOO
  #if BAR
    #include <foo>
  #endif
#endif

[drashna]

Personally, I don't have a preference one way or the other. I just want consistency :)

[XScorpion2]

I lean towards None or Before Hash. After Hash is confusting to look at if you don't have a good c syntax highlighting.

[XScorpion2]

For none, I do usually expect a comment on the #endif line stating which #if it was for:

endif // BAR

[drashna]

For none, I do usually expect a comment on the #endif line stating which #if it was for:

endif // BAR

100% this.

[noroadsleft]

I have a slight preference for BeforeHash above AfterHash. None is a complete non-starter for me unless the #endif has a comment indicating which directive is terminating, as @XScorpion2 said.

[zvecr]

Just incase anyone wants a more real world example of BeforeHash https://github.com/zvecr/qmk_firmware/tree/future_unformatted_before

My order of preference would be BeforeHash then None and then way down AfterHash

[vomindoraan]

I really don't like AfterHash for the simple reason that, other than looking odd, it makes code harder to grep for. I'm used to adding \s* after every # in my regexes, but it'd be great if I didn't have to do this, and not everyone will remember to do it anyway.
None is a bit better, but not ideal either, as it can make complex preprocessor logic (which there's a lot of in QMK, due to feature flagging etc.) hard to understand and parse visually.
BeforeHash is the only viable option for the long term, imo. As an added benefit, it makes the #endif comments unnecessary, which is good since those may be hard to enforce anyway.

Another thing worth discussing is whether preprocessor directive indentation should affect the indentation of regular code. In my opinion the answer is no, and I believe clang-format will format code like this by default. Here are some examples of how preprocessor indentation can affect the readability of code.

[vomindoraan]

In summary, I think we should go with BeforeHash + preprocessor directives having their own, separate indentation levels in most cases. The exception is when following regular indentation clearly makes a piece of code more readable (readability over consistency). However, I'm not sure if it's possible to set up clang-format to allow for this (and it might be worthwhile to stay totally consistent, anyway).

[fauxpark]

Agree with @vomindoraan - BeforeHash with separate indentation as much as possible. But until that option is more widespread perhaps AfterHash is best so there is at least some form of indentation.

[vomindoraan]

I believe BeforeHash is currently the most widespread option in the repo in general. As far as core code is concerned, we could always reformat all of it in this round of breaking changes.

[nooges]

My preference is None, but I’m okay with BeforeHash

[nooges]

@vomindoraan I’m with you on that preprocessir directives should not affect the indentation of regular code.

[yiancar]

I am for BeforeHash as well. Some people like to add a comment at the endif in those cases as well, I dont mind that TBH

[skullydazed]

It's clear that long term BeforeHash is the preferred setting. We will need to monitor clang-format's propagation to determine when we can use that.

In the meantime it seems like we can choose AfterHash or None. If we choose None clang-format is not able to enforce #end comments. Given that, which setting makes sense until we can use BeforeHash?

[vomindoraan]

My vote goes to AfterHash until Clang 9 is more widely available. I think having at least some indentation is better than having none, since we can't enforce the ending comments. We'll just have to get used to taking the spaces into account when searching through code for the time being.

[ishtob]

also vote BeforeHash

[skullydazed]

Thanks for your comments everyone. For now we will use AfterHash, and we will revisit switching to BeforeHash at each breaking change window until clang-format 9 has enough adoption.