969847 - Simplify the proxy return classes used to ensuring that the fallible array's SetCapacity returns bool but the infallible array's SetCapacity returns void

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[(no longer active)]

Benoit came up with a much better way of implementing bug 853548:

(In reply to Benoit Jacob [:bjacob] from bug 853548 comment #17)
> Indeed, two things:
> 
> 
>  1. You can use MFBT's mozilla::Conditional to define the return type:
> 
>    typename mozilla::Conditional<IsFallible, bool, void>::Type
> 
>   (as usual, 'typename' is needed to get at member typedefs in types
> depending on template parameters).
> 
> 
>  2. You then don't need to wrap the return value in any kind of proxy class,
> you can simply return true or false cased to the type that you obtained at
> step 1.
> 
> 
> Putting this together, here is a toy nsTArray::SetCapacity():
> 
> template<typename T, bool IsFallible>
> typename mozilla::Conditional<IsFallible, bool, void>::Type
> nsTArray<T, IsFallible>::SetCapacity(size_t newCapacity)
> {
>   bool success = do_work(newCapacity);
>   return typename mozilla::Conditional<IsFallible, bool,
> void>::Type(success);
> }
> 
> You could evaluate once and for all that awkward Conditional expression in a
> member typedef in nsTArray.

There is one detail missing, we still need the Alloc::Successful() idiom to be able to use those return values to test for truthfulness inside the templated code, but all of the rest of the hacks I did in that bug can be removed!

Comment 1

[(no longer active)]

Attachment: Simplify the proxy return classes used to ensuring that the fallible array's SetCapacity returns bool but the infallible array's SetCapacity returns void; r=froydnj

Comment 2

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=69e74b353ca0

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8372831 [details] [diff] [review]
Simplify the proxy return classes used to ensuring that the fallible array's SetCapacity returns bool but the infallible array's SetCapacity returns void; r=froydnj

Review of attachment 8372831 [details] [diff] [review]:
-----------------------------------------------------------------

r- for burning try and even crashing clang.  try appears to not like returning void from various places like EnsureCapacity.

::: xpcom/glue/nsTArray.h
@@ +105,2 @@
>  
> +  static bool Successful(...) {

Why not just bool?  Trying to avoid unused parameter warnings?  varargs is very weird to see here.

I think we want to MOZ_ASSERT the result parameter here anyway.

Comment 4

[(no longer active)]

(In reply to Nathan Froyd (:froydnj) from comment #3)
> ::: xpcom/glue/nsTArray.h
> @@ +105,2 @@
> >  
> > +  static bool Successful(...) {
> 
> Why not just bool?  Trying to avoid unused parameter warnings?  varargs is
> very weird to see here.

So, the argument passed to this function is of type void.  This is one way of making sure that code of this pattern:

  if (!Alloc::Successful(ThingWhichReturns_result_type_WhichCanBeVoidFor_nsTArray()))
    return false;

will work.

And it does work as expected in my local debug builds with clang.

But it seems to me like we're getting ahead of the state of the art in compiler writing.

Some day we will have compilers which can compile C++.  Until then, WONTFIX seems like the only viable option. :(