Closed Bug 1298412 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Disability Access APIs, defect)

All
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: froydnj, Assigned: aklotz)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch Fix (obsolete) — Splinter Review
Assignee: nobody → aklotz
Status: NEW → ASSIGNED
Attachment #8785355 - Flags: review?(tbsaunde+mozbugs)
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 ;-)
Attachment #8785355 - Attachment is obsolete: true
Attachment #8785355 - Flags: review?(tbsaunde+mozbugs)
Attachment #8785397 - Flags: review?(jmathies)
Fixed one more function signature.
Attachment #8785397 - Attachment is obsolete: true
Attachment #8785397 - Flags: review?(jmathies)
Attachment #8785399 - Flags: review?(jmathies)
(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 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.
Attachment #8785399 - Flags: review?(jmathies) → review+
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
https://hg.mozilla.org/mozilla-central/rev/fba5313b6782
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.