Re: NotNull and pointer parameters that may or may not be null
On Mon, Jul 22, 2019 at 10:00 AM Karl Tomlinson wrote: > 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] I understand that there's value in adopting Google rules without modification in order to avoid having to discuss which parts to adopt and to avoid having to adapt tooling in accordance to the results of such discussion. However, what should we think of Google disagreeing with the C++ Core Guidelines, which can be expected to also have larger ecosystem value as a tooling target and that in some sense could be considered to come from higher C++ authority than Google's rules? On this particular topic, the Core Guidelines have: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-inout https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#f60-prefer-t-over-t-when-no-argument-is-a-valid-option https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#f23-use-a-not_nullt-to-indicate-that-null-is-not-a-valid-value (FWIW, I have relatively recently r+ed code that used non-const references to indicate non-nullable modifiable arguments on the grounds that it was endorsed by the Core Guidelines.) -- Henri Sivonen hsivo...@mozilla.com ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: NotNull and pointer parameters that may or may not be null
FWIW RefPtr already behaves as MaybeNull for assert-on-deref, making them functionally Maybe. (though they don't represent a contract that it's nullable) For non-outvar-args, I tend to use references to document non-null, though I do so even for many non-const uses. (balancing what is and isn't `mutable`/non-const is hard. Casual vs important side-effects) References obviously should not be null, which makes the non-null contract pretty obvious, though there's no assert for `*(T*)foo`. Like you say though, null-derefs are self-asserting. :) Outvars are always pointers for me, non-null unless documented otherwise. (I should probably be using NotNull, but again, self-asserting) I agree that it's often not clear when args are actually nullable, with the exception of dom::Nullable coming out of bindings code. Maybe we should move Nullable to mfbt? I like the idea of annotating these contracts. (Possibly also a NullableRefPtr? I occasionally use Maybe> for this, but the double-deref it requires is sort of awkward) If we want a pure annotation, we could always `#define NULLABLE`! On Mon, Jul 22, 2019 at 12:00 AM Karl Tomlinson wrote: > > 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 ___
NotNull and pointer parameters that may or may not be null
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
