https://google.github.io/styleguide/cppguide.html#Reference_Arguments has a simple rule to determine when reference parameters are permitted: "Within function parameter lists all references must be const." This is consistent with Mozilla's previous coding style: "Use pointers, instead of references for function out parameters, even for primitive types." [1] However, across Gecko there are different interpretations of what "out" parameter means.
The Google style considers a parameter to be an out parameter if any of its state may be mutated by the callee. In some parts of Gecko, a parameter is considered an out parameter only if the callee might make wholesale changes to the state of parameter. Well before the announcement to switch to Google style, this interpretation was discussed in 2017 [2], with subsequent discussion around which types were suitable as non-const reference parameters. I'm asking how should existing surrounding code with some different conventions affect when is it acceptable to follow Google style for reference or pointer parameters? The announcement of changes to Mozilla style [3] says > Google C++ style will be permitted but not initially enforced, > and consistency with surrounding code should take precedence. and > we have decided that by allowing the usage of the Google C++ > Style in new areas, we are taking an incremental step towards > that ultimate goal [Google C++ Style)] by allowing (not > requiring) people to write code immediately after our switch > that is consistent with our newly adopted coding style [...] > The intention here is not that people intermix Google C++ Style > and Mozilla Style code on a per function or even a per file > basis but rather that they be free to do so when starting new, > self-contained work, or to convert an entire module. Parameters cross module boundaries. For example, WebIDL bindings use non-const references and pointers to distinguish between C++ representations of non-nullable and nullable interface and callback types [4]. Classes of many modules need to break from Google style to provide appropriate calling conventions to WebIDL bindings. Should this mean that such classes need to be consistent with the WebIDL binding convention when declaring other methods just because they need to implement some methods for WebIDL bindings? If conventions should traverse modules boundaries in this way, then very little new code would follow the new style. Is that the expectation or is it preferable to aim to follow the new style where possible? A recent post [5] appears to propose a slightly different interpretation: > In other words, we should default to using Google C++ style when > doing so would not be massively more disruptive or inconsistent > than the alternative. So we're not boiling the ocean over mFoo, > but preferring the explicit integer types and citing the Google > style guide is a reasonable thing to do. Declaring new methods to follow Google parameter conventions is not in general massively more disruptive or even inconsistent than the alternative. Even when working with similar types, the mix of pointer/reference usage and the existence of other situations where references are not suitable [6] means that conversions between references and pointers are already regularly required. I can imagine reasons why the reference/pointer convention between nullable and non-nullable types is something that may have been particularly useful for implementing WebIDL interfaces but that distinction is not necessarily the best reference/pointer convention in general, and so choices made for WebIDL bindings should not necessarily influence other code. Consistency is good, but that argument can be applied in either direction. There has been similar conflict between reference parameter conventions in Blink. Google and Chromium styles forbid mutable reference parameters. WebKit introduced mutable reference parameters to help in wiping out phantom null checks [7] but this change was not completed (in Blink at least). Code that has moved from Chromium into Blink has not changed style [8] and blink/core/layout/ng has chosen Google style rather than legacy WebKit style so as to highlight side effects in callees [9]. People using mutable reference parameters are typically pleased with their choice, and people using only const reference parameters are pleased with their choice [10]. Blink style was recently (March 20) changed to accept either style, to reflect reality [11]. No single choice between the conventions has yet been made for Blink. When reading over a call site and surrounding code, I find knowledge of mutability of arguments necessary first for high level understanding. Missing a mutation often means completely misunderstanding. Whether arguments may be null or not is usually a detail secondary in understanding of the function of the code. Disclaimer: I was asked by a reviewer to post about this topic, but this post has my own opinions. [1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#COM_and_pointers [2] https://groups.google.com/forum/#!msg/mozilla.dev.platform/_jfnwDvcvN4/HozN0qLhCAAJ [3] https://groups.google.com/forum/#!msg/mozilla.dev.platform/VCLB1Lz4yXc/dNoNVzmlCQAJ [4] https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#typemapping [5] https://groups.google.com/d/msg/mozilla.dev.platform/ekmfMjjF7Mg/h8Bov4MEAwAJ [6] https://searchfox.org/mozilla-central/rev/da855d65d1fbdd714190cab2c46130f7422f3699/mfbt/NotNull.h#32-60 [7] https://groups.google.com/a/chromium.org/d/msg/blink-dev/O7R4YwyPIHc/F1_9Y_hGEAAJ [8] https://groups.google.com/a/chromium.org/d/msg/blink-dev/O7R4YwyPIHc/NbzaUcG4AQAJ [9] https://groups.google.com/a/chromium.org/d/msg/blink-dev/O7R4YwyPIHc/gHKIqsudAAAJ [10] https://groups.google.com/a/chromium.org/d/msg/blink-dev/O7R4YwyPIHc/zXffEnzaAAAJ [11] https://groups.google.com/a/chromium.org/d/msg/blink-dev/O7R4YwyPIHc/1SXD1UCOCAAJ _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
