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

RESOLVED WONTFIX

Status

()

RESOLVED WONTFIX
5 years ago
5 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

Trunk
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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!
(Assignee)

Comment 1

5 years ago
Created 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
(Assignee)

Updated

5 years ago
Attachment #8372831 - Flags: review?(nfroyd)
(Assignee)

Updated

5 years ago
Blocks: 969864
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.
Attachment #8372831 - Flags: review?(nfroyd) → review-
(Assignee)

Comment 4

5 years ago
(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. :(
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
(Assignee)

Updated

5 years ago
No longer blocks: 969864
You need to log in before you can comment on or make changes to this bug.