FWIW RefPtr already behaves as MaybeNull for assert-on-deref, making them functionally Maybe<T&>. (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<RefPtr<T>> 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 <mozn...@karlt.net> 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 _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
