Google style requires pointers for parameters that may be mutated by the callee, which provides that the potential mutation is visible at the call site. Pointers to `const` types are permitted, but recommended when "input is somehow treated differently" [1], such as when a null value may be passed.
Comments at function declarations should mention "Whether any of the arguments can be a null pointer." [2] Some Gecko code has instead sometimes used a different reference/pointer convention to identify parameters which can be null. With Google style, there is the question of whether to use a different mechanism to indicate whether or not pointer parameter may be null. If a method signature wants a pointer to an object of a particular type, then I would usually expect to need to provide a pointer to an object, unless documented otherwise. I wonder whether in the general case, possibly-null pointers are more the exception than the rule? Does use of NotNull add much value if most pointer parameters are assumed not null, but not explicitly wrapped? I wonder whether the reverse kind of wrapping would be more useful? A MaybeNull type was previously contemplated but considered more complex and less obviously useful [3]. A NotNull wrapping of the parameter would catch nulls when set in debug builds. A MaybeNull wrapping could catch nulls when dereferenced. Assertions don't really seem necessary to me because we are almost certain to find out anyway if code that expects non-null receives a null pointer. Behavior is actually undefined, but I don't recall ever seeing a small null offset dereference bug rated worse than sec-low. The primary benefit of a wrapping would be to annotate expectations and perhaps even require more though about what those are. mozilla::NotNull was introduced in 2016 [4] but hasn't seen much adoption. I found very few header files that use NotNull consistently, but looked at the ones with most usage. At the time that NotNull usage was introduced to image/DecoderFactory.h [5], it was applied to eight parameters. Two pointer parameters did not get wrapped with NotNull. Of those the `const char* aMimeType` parameter on the public method was assumed non-null by the implementation. The other parameter was for a private method that permits `RasterImage* aImage` to be null. Since then, two additional `IDecodingTask** aOutTask` pointer parameters have been added, each of which is assumed non-null. intl/Encoding.h [6] has seven uses of NotNull in return types. In the C++ API exposed, I found five instances of pointers in return values where NotNull was not applied: each used nullptr as a sentinel. It seems that NotNull is particularly useful for Encoding because sometimes there may be a sentinel value and other times not. A MaybeNull alternative would be at least as useful in this case. I'm inclined to skip NotNull unless there is a special reason to use it. Anyone have reason to feel differently? [1] https://google.github.io/styleguide/cppguide.html#Reference_Arguments [2] https://google.github.io/styleguide/cppguide.html#Function_Comments [3] https://groups.google.com/d/msg/mozilla.dev.platform/_jfnwDvcvN4/rl6c3TcFAQAJ [4] https://groups.google.com/d/msg/mozilla.dev.platform/6M_afibWhBA/MCmrYktaBgAJ [5] https://searchfox.org/mozilla-central/rev/f6528fc8520d47e507877da3dda798ab57385be2/image/DecoderFactory.h#93 [6] https://searchfox.org/mozilla-central/rev/4fbcfe6fecf75d7b828fbebda79a31385f7eab5f/intl/Encoding.h _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
