prohibit expr ? mSmartPointer : nullptr -style patterns with T* result

NEW
Unassigned

Status

defect
4 years ago
Last year

People

(Reporter: froydnj, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

They lead to inefficient code and require implicit conversions to T* from smart pointer temporaries, which are undesirable.
Would you be wanting to catch these with a separate static analysis, or just as part of landing Bug 1179451?
(In reply to Michael Layzell [:mystor] from comment #1)
> Would you be wanting to catch these with a separate static analysis, or just
> as part of landing Bug 1179451?

Hm.  I guess the static analysis becomes redundant if bug 1179451 lands, doesn't it?

I had thought it might be easier to do the static analysis work (and the corresponding small efficiency win) in a separate bug, regardless of whether we can get the ref-qualifier bits in bug 1179451 landed.  But maybe that's not the case...WDYT?
Flags: needinfo?(michael)
Yeah, the static analysis will be redundant with bug 1179451.

I think the question of whether or not the static analysis is worthwhile is pretty much based on how difficult it will be to land 1179451. If you think it will be very difficult to get all of the changes in to land 1178451 within the nearish future, then a static analysis will be reasonable, but it seems a bit silly to put one in if it's going to be ripped out in a couple of weeks/months.
Flags: needinfo?(michael)
As I pointed out on dev.platform, I *think* compilers are allowed to optimize away this temporary per spec, per 12.8.32 of <http://www.open-std.org/Jtc1/sc22/wg21/docs/papers/2011/n3242.pdf>, third item in list.  So if the AddRef/Release is the issue here, it would be a good idea to check whether the compilers we use actually produce it.
Hunh, I think you may be right, I didn't realize that these copies were permitted to be optimized out even if there were side effects. Given the spec which you have linked, I will believe that the temporary is allowed to be optimized away. We probably should check though.
clang (3.5?) and gcc (4.9) do not perform copy-elision in this case.

clang is smart enough to do it if you write the code like this (nsPluginFrame::GetWidget()) (!):

  nsCOMPtr<nsIWidget> np(nullptr);
  return mInnerView ? mWidget : np;

but not if you write it like:

  return mInnerView ? mWidget : nsCOMPtr<nsIWidget>(nullptr);

Haven't looked at MSVC.
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #6)
> clang (3.5?) and gcc (4.9) do not perform copy-elision in this case.
> 
> clang is smart enough to do it if you write the code like this
> (nsPluginFrame::GetWidget()) (!):
> 
>   nsCOMPtr<nsIWidget> np(nullptr);
>   return mInnerView ? mWidget : np;
> 
> but not if you write it like:
> 
>   return mInnerView ? mWidget : nsCOMPtr<nsIWidget>(nullptr);
> 
> Haven't looked at MSVC.

Clang isn't "smart enough" to remove the copy in that situation, rather the copy simply doesn't happen in that case. 

The reason why the copy is necessary in the original case was because the nsCOMPtr for the nullptr was stored in a temporary, and thus the value of the ternary had to be an xvalue, but mWidget was an lvalue, and thus had to be copied into a temporary to make it an xvalue as well. If you make the nullptr side not be an xvalue, but instead an lvalue (by moving it into a named variable), then suddenly the lvalue -> xvalue conversion is unnecessary, and thus the copy never occurs.

So I guess compilers today are just not clever enough to avoid this copy (or there is other fine print which would say that it is illegal/unsound in this situation).
The standard's description of the ternary operator seems to contradict your description of the copy not taking place:

"The second and third operands have the same type; the result is of that type. If the operands have
class type, the result is a prvalue temporary of the result type, which is copy-initialized from either
the second operand or the third operand depending on the value of the first operand."
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #8)
> The standard's description of the ternary operator seems to contradict your
> description of the copy not taking place:
> 
> "The second and third operands have the same type; the result is of that
> type. If the operands have
> class type, the result is a prvalue temporary of the result type, which is
> copy-initialized from either
> the second operand or the third operand depending on the value of the first
> operand."

That doesn't seem to agree with what clang does. In fact, clang accepts code like this:

int main() {
  Foo a(1);
  Foo b(2);
  (true ? a : b) = Foo(3);
}

which suggests that a ternary can produce an lvalue, rather than only a prvalue temporary. The generated AST actually names the ConditionalOperator as `lvalue`.

My guess is that lvalue vs xvalue vs rvalue etc are part of the type of the value. If the type is the same, as the first line says, the result is of that type. If the type is different, then it checks if it is a class type, and produces the prvalue temporary if it is.

Nonetheless, no copy is produced in the situation where both sides of the ternary are lvalues.
As bug 1179451 won't be able to land because it isn't supported on the versions of MSVC and gcc which we support, I think a static analysis like this would be useful.

I'm planning to write a rewriter tool to correct this pattern by adding the call to get(), and then introducing a static analysis to prevent new code with this pattern from being added in the future.
Assignee: nobody → michael
Aryeh has expressed interest on working on this and bug 1179451, so I'm going to leave this to him.
Assignee: michael → ayg
I'll look at bug 1179451, and if I can fix that, this should be fixed as well.  But I don't intend to look at this bug separately -- I've never looked at our static analysis stuff before.
Assignee: ayg → nobody
(In reply to Michael Layzell [:mystor] from comment #9)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #8)
> > The standard's description of the ternary operator seems to contradict your
> > description of the copy not taking place:
> > 
> > "The second and third operands have the same type; the result is of that
> > type. If the operands have
> > class type, the result is a prvalue temporary of the result type, which is
> > copy-initialized from either
> > the second operand or the third operand depending on the value of the first
> > operand."
> 
> That doesn't seem to agree with what clang does. In fact, clang accepts code
> like this:
> 
> int main() {
>   Foo a(1);
>   Foo b(2);
>   (true ? a : b) = Foo(3);
> }
> 
> which suggests that a ternary can produce an lvalue, rather than only a
> prvalue temporary. The generated AST actually names the ConditionalOperator
> as `lvalue`.
> 
> My guess is that lvalue vs xvalue vs rvalue etc are part of the type of the
> value. If the type is the same, as the first line says, the result is of
> that type. If the type is different, then it checks if it is a class type,
> and produces the prvalue temporary if it is.
> 
> Nonetheless, no copy is produced in the situation where both sides of the
> ternary are lvalues.

Interpreting the standard can be a subtle business at times :)

The relevant paragraphs of [expr.cond] are:

=============================================================================
  4. If the second and third operands are glvalues of the same value category 
     and have the same type, the result is of that type and value category ...
     <snip>
 
  5. Otherwise, the result is a prvalue. If ... <snip>. Otherwise, the 
     conversions thus determined are applied, and the converted operands are 
     used in place of the original operands for the remainder of this section.

  6. Lvalue-to-rvalue (4.1), array-to-pointer (4.2), and function-to-pointer (4.3) 
     standard conversions are performed on the second and third operands. After 
     those conversions, one of the following shall hold:

       — The second and third operands have the same type; the result is of that type. 
         If the operands have class type, the result is a prvalue temporary of the 
         result type, which is copy-initialized from either the second operand or the 
         third operand depending on the value of the first operand.
=============================================================================

The passage Nathan quoted is from paragraph 6. However, paragraph 5 says that the remainder of the section (including paragraph 6) applies "otherwise", i.e. if preceding paragraphs didn't apply. In this case, paragraph 4 applies, and that tells us the result is an lvalue (and thus no copy is made), so we don't reach paragraphs 5 or 6.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.