1298412 - clang-cl error: rvalue passed to non-const reference in CreateInterceptor

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Nathan Froyd [:froydnj]]

4:27.76 c:/m-c/accessible/windows/msaa/RootAccessibleWrap.cpp(62,16):  error: no matching function for call to 'CreateInterceptor'
 4:27.76   HRESULT hr = CreateInterceptor<IAccessible>(
 4:27.76                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 4:27.76 C:/m-c/obj-clang-cl-optimized/dist/include\mozilla/mscom/interceptor.h(107,1):  note: candidate function [with InterfaceT = IAccessible] not viable: expects an l-value for 1st argument
 4:27.76 CreateInterceptor(STAUniquePtr<InterfaceT>& aTargetInterface,
 4:27.76 ^

The code in question looks like:

  HRESULT hr = CreateInterceptor<IAccessible>(
      STAUniquePtr<IAccessible>(rootAccessible.forget().take()), eventSink,
      getter_AddRefs(interceptor));

and the function prototype looks like:

template <typename InterfaceT>
inline HRESULT
CreateInterceptor(STAUniquePtr<InterfaceT>& aTargetInterface,
                  IInterceptorSink* aEventSink,
                  InterfaceT** aOutInterface)

so we're passing a temporary STAUniquePtr to a STAUniquePtr&, which isn't allowed.  If the parameter was a const STAUniquePtr&, that would be OK, but seeing as how CreateInterceptor wants to take ownership, we can't use const here.

We need to store the STAUniquePtr in a named variable before passing it to CreateInterceptor.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Fix

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Change pass-by-reference to Move+pass-by-value

Trevor pointed out that it makes more sense to just make these functions accept STAUniquePtr by value and use Move(). It also clarifies the intent of the caller.

Plus it allows him to get out of doing this review, since it no longer touches a11y code ;-)

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Change pass-by-reference to Move+pass-by-value (r2)

Fixed one more function signature.

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Aaron Klotz [:aklotz] from comment #2)
> Trevor pointed out that it makes more sense to just make these functions
> accept STAUniquePtr by value and use Move(). It also clarifies the intent of
> the caller.

The only concern I'd have about that is since CreateInterceptor can fail, defining the interface that way means that on failure, the STAUniquePtr is destroyed, which may or may not be helpful.  But since there's only one caller (?), perhaps that's not such a big deal--and we have lots of other cases where we pass UniquePtrs by value into functions that have similar failure problems.

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Nathan Froyd [:froydnj] from comment #4)
> (In reply to Aaron Klotz [:aklotz] from comment #2)
> > Trevor pointed out that it makes more sense to just make these functions
> > accept STAUniquePtr by value and use Move(). It also clarifies the intent of
> > the caller.
> 
> The only concern I'd have about that is since CreateInterceptor can fail,
> defining the interface that way means that on failure, the STAUniquePtr is
> destroyed, which may or may not be helpful.  But since there's only one
> caller (?), perhaps that's not such a big deal--and we have lots of other
> cases where we pass UniquePtrs by value into functions that have similar
> failure problems.

In this case I decided that it was acceptable because the failure paths of those functions can't do anything other than destroy the pointer. In this context we always have to either succeed or just destroy it.

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fba5313b6782031568c7197d2de433e2dfc924aa
Bug 1298412: Fix Interceptor construction to use Move semantics for STAUniquePtr instead of pass-by-reference; r=jimm

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fba5313b6782