Proposal to adjust our clang-format rules to include spaces after the hash for nested preprocessor directives

[Ehsan Akhgari]

Hi everyone,

I'd like to propose the first modification to our clang-format rules based
on the feedback I've received since the switch to our new coding style from
the SpiderMonkey team.

The problem we're trying to solve is that in code that has nested
preprocessor directives, when the directives are far apart from each other,
it may be difficult to spot that a given directive is nested under another
one, and this may hide important logic in cases such as a chain of
#if/#ifdef directives. Here is a practical example: <
https://github.com/sylvestre/gecko-dev/blob/master/js/src/wasm/WasmBaselineCompile.cpp#L4054>.
When looking at code like this, it can be quite difficult to know where in
the (potential) preprocessor nesting levels you are without scrolling
around quite a bit and keeping a lot of context in mind. In the example
above, I would say that's not reasonably possible to do.

The common way to deal with this problem is to indent nested preprocessor
directives, very much similarly to how we indent normal code, for example:

#if foo
# if bar
# define x 1
# else
# define x 2
# endif
#endif

This is allowed, but not required, by our coding style <
https://google.github.io/styleguide/cppguide.html#Preprocessor_Directives>,
and is supported by clang-format by using the IndentPPDirectives: AfterHash
option <https://clang.llvm.org/docs/ClangFormatStyleOptions.html>. So in
order to accommodate easier reading of code like this, I propose to adopt
that clang-format option.

You can look at how the above code looks like after reformatting the tree
with that option here: <
https://github.com/ehsan/gecko-dev/blob/45df04c211f4dd875c58125bca5fbca381ed8fca/js/src/wasm/WasmBaselineCompile.cpp#L4824>.
If you are curious to look at the entire tree or the resulting changes,
they're available for viewing here respectively: <
https://github.com/ehsan/gecko-dev/tree/afterhash> and <
https://github.com/ehsan/gecko-dev/commit/45df04c211f4dd875c58125bca5fbca381ed8fca
>.

Please let me know if you have any suggestions or concerns. Ideas that
have come up before include doing nothing about this problem (which I think
isn't a reasonable answer since I think the problem proposed is valid and
has a viable solution covered in our coding style which we can achieve
easily with tooling) and doing this in a small scope, for example only for
SpiderMonkey (which the SpiderMonkey team or myself prefer not to do since
we don't have any evidence to suggest the problem is localized to that part
of the code base and we prefer to not have special cases in our coding
style where we can avoid it, also this change isn't nearly as invasive as
the tree-wide reformat so we should be able to do it with minimal
disruption to anyone's work).

I'm planning to file a bug on this by the end of next week if no serious
concerns aren't raised by then.

Thanks,
--
Ehsan

[Kris Maglione]

On Thu, Jan 10, 2019 at 08:01:52PM -0500, Ehsan Akhgari wrote:
>The common way to deal with this problem is to indent nested preprocessor
>directives, very much similarly to how we indent normal code, for example:
>
>#if foo
># if bar
># define x 1
># else
># define x 2
># endif
>#endif

+1

This has been my preferred style for a long time, but lately
clang-format just throws it away whenever I try to do it.

[Gabriele Svelto]

On 11/01/19 02:01, Ehsan Akhgari wrote:
> The common way to deal with this problem is to indent nested preprocessor
> directives, very much similarly to how we indent normal code, for example:
>
> #if foo
> # if bar
> # define x 1
> # else
> # define x 2
> # endif
> #endif

this would be much better than what we have now and I recently ran into
an issue with a complex block of #ifdef's that I would have spotted
easily if they had been indented. The first iteration of a patch I wrote
"accidentally" cut out a chunk of a function when built on Windows but
still compiled and even executed most of our test suite just fine.
Fortunately a couple of xpchsell tests failed, but if it hadn't been
covered by those I might not have spotted the error at all.

Gabriele

[Ehsan Akhgari]

I've received some on and off-list responses to this, all in favour. I've
filed https://bugzilla.mozilla.org/show_bug.cgi?id=1521000 to make the
change.

Thanks to everyone who provided feedback.

On Thu, Jan 10, 2019 at 8:01 PM Ehsan Akhgari <ehsan....@gmail.com>
wrote:

> Hi everyone,
>
> I'd like to propose the first modification to our clang-format rules based
> on the feedback I've received since the switch to our new coding style from
> the SpiderMonkey team.
>
> The problem we're trying to solve is that in code that has nested
> preprocessor directives, when the directives are far apart from each other,
> it may be difficult to spot that a given directive is nested under another
> one, and this may hide important logic in cases such as a chain of
> #if/#ifdef directives. Here is a practical example: <
> https://github.com/sylvestre/gecko-dev/blob/master/js/src/wasm/WasmBaselineCompile.cpp#L4054>.
> When looking at code like this, it can be quite difficult to know where in
> the (potential) preprocessor nesting levels you are without scrolling
> around quite a bit and keeping a lot of context in mind. In the example
> above, I would say that's not reasonably possible to do.
>

> The common way to deal with this problem is to indent nested preprocessor
> directives, very much similarly to how we indent normal code, for example:
>
> #if foo
> # if bar
> # define x 1
> # else
> # define x 2
> # endif
> #endif
>

--
Ehsan