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