On Tue, May 09, 2017 at 06:24:56PM +0300, smaug wrote: > On 05/09/2017 04:52 PM, smaug wrote: > > On 05/09/2017 01:55 PM, Mike Hommey wrote: > > > On Tue, May 09, 2017 at 01:31:33PM +0300, Henri Sivonen wrote: > > > > On Tue, May 9, 2017 at 12:58 PM, Emilio Cobos Álvarez > > > > <emi...@crisal.io> wrote: > > > > > I think references help to encode that a bit more in the type system, > > > > > and help reasoning about the code without having to look at the > > > > > implementation of the function you're calling into, or without having > > > > > to > > > > > rely on the callers to know that you expect a non-null argument. > > > > > > > > > > Personally, I don't think that the fact that they're not used as much > > > > > as > > > > > they could/should is a good argument to prevent their usage, but I > > > > > don't > > > > > know what's the general opinion on that. > > > > > > > > The relevant bit of the Core Guidelines is > > > > https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rf-ptr-ref > > > > and says: > > > > "A pointer (T*) can be a nullptr and a reference (T&) cannot, there is > > > > no valid "null reference". Sometimes having nullptr as an alternative > > > > to indicated "no object" is useful, but if it is not, a reference is > > > > notationally simpler and might yield better code." > > > > > > > > As a result, I have an in-flight patch that takes T& instead of > > > > NotNull<T*> where applicable, even though I do use NotNull<T*> to > > > > annotate return values. > > > > > > > > I agree that in principle it makes sense to use the type system > > > > instead of relying on partial debug-build run-time measures to denote > > > > non-null arguments when possible. That said, having to dereference a > > > > smart pointer with prefix * in order to pass its referent to a method > > > > that takes a T& argument feels a bit odd when one is logically > > > > thinking of passing a pointer still, but then, again, &*foo seems like > > > > common pattern on the Rust side of FFI to make a reference out of a > > > > pointer and effectively asserting to the human reader that the pointer > > > > is null. > > > > > > Note that if you dereference a pointer to pass as a reference, all hell > > > breaks loose and your reference might just as well be null, but the > > > function taking the reference won't be protected against it because the > > > compiler will have assumed, when compiling it, that the reference can't > > > be null. > > > > > This is what I'm a bit worried. We're moving some null checks from callee > > to caller, so need to > > learn to be more careful with null checks on caller side. > > But in general, using references sounds ok. > > > > > I could add still, that using references does add another thing to reviewers' > checklist to ensure > that null pointer isn't ever dereferenced, and it also means that there will > be more null checks, since > parameters need to be validated on callers' side, not in callee.
To be clear, I'm not suggesting to move functions that now do the null-checking in the function itself to take references. I think functions that take pointers and handle null are perfectly fine. But doing functions that already don't null-check because they're already checked at the callsite seem to me they could perfectly take a reference. Also, there's a case for the opposite I think, where taking references make reviewing easier. To give one example, I just grepped a bit and found EffectCompositor::GetAnimationElementAndPseudoForFrame[1] (there are a lot of other functions like that, like all the IsFoo functions in the frame constructor, etc). In any case: That function dereferences the aFrame parameter without null-checking, not even asserting. If someone is, hypothetically, adding a caller to that function, and calls it with a frame pointer that may be null, I think that function taking a reference would, in any case, make the job of the reviewer _easier_: It makes the explicit dereference at the callsite, so the reviewer can evaluate whether that dereference is correct. Otherwise, the reviewer would need to check the implementation of GetAnimationElementAndPseudoForFrame in order to realise that it doesn't handle null. It's kind of a silly example (because that function itself doesn't make a lot of sense if you don't give it a frame), but I can find some more realistic examples of the same situation if you want to. -- Emilio [1]: http://searchfox.org/mozilla-central/source/dom/animation/EffectCompositor.cpp#704
signature.asc
Description: PGP signature
_______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
