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.akhg...@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 > > 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 > -- Ehsan _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
