fix clang errors about copying jni::StringParam in PrefsHelper.h

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: froydnj, Unassigned)

Tracking

Trunk
mozilla50
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 affected, firefox50 fixed)

Details

Attachments

(1 attachment)

For this code and other instances like it in PrefsHelper.h:

        const auto& jstrVal = type == widget::PrefsHelper::PREF_STRING ?
                jni::StringParam(strVal, aPrefName.Env()) :
                jni::StringParam(nullptr);

clang says:

/home/froydnj/src/gecko-dev.git/widget/android/PrefsHelper.h:71:17: error: call to implicitly-deleted copy constructor of 'jni::StringParam'
                jni::StringParam(strVal, aPrefName.Env()) :
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/opt/build/froydnj/build-android/dist/include/mozilla/jni/Refs.h:635:21: note: copy constructor of 'StringParam' is implicitly deleted because base class 'String::Ref' (aka 'Ref<mozilla::jni::TypedObject<jstring>, _jstring *>') has an inaccessible copy constructor
class StringParam : public String::Ref
                    ^

From reading through the code and comments, a deleted copy constructor looks like an explicit design choice.  clang is following the letter of the spec here; I think GCC is either getting name lookup completely wrong, or doing something like:

  char data[sizeof(StringParam)];
  if (type == TYPE_STRING) {
    new (&data) StringParam(...);
  } else {
    new (&data) StringParam(nullptr);
  }
  const auto& jstrVal = reinterpret_cast<...>(...);

which would be a little tricky, but totally doable.  I have tried to rewrite things in such a way as to ensure that we don't do any extra work for the non-TYPE_STRING case, but I am stumped.  The best idea I have come up with involves mozilla::Maybe:

  Maybe<const StringParam> jstrVal;
  jstrVal.emplace(nullptr);
  if (type == TYPE_STRING) {
    jstrVal.reset();
    jstrVal.emplace(...);
  }

but that is ugly, although it takes care of any necessary destructors for us.  Do you have any better ideas?
Flags: needinfo?(nchen)
In an expression such as:

  const auto& x = cond() ? AClass(...) : AClass();

the C++ standard specifies that the copy constructor of AClass is
invoked on the result of the conditional expression ([expr.cond]p6).
GCC does not honor this part of the specification, whereas clang does;
clang therefore complains about instances of code such as:

   const auto& jstrVal = type == widget::PrefsHelper::PREF_STRING ?
       jni::StringParam(strVal, aPrefName.Env()) :
       jni::StringParam(nullptr);

as jni::StringParam is not copy-constructable.

The simplest solution that does not introduce unnecessary allocation
uses mozilla::Maybe to hold the temporary objects and to hide some of
the details of constructing objects in-place.  The compiler may even be
able to optimize away some of the unnnecessary checks that Maybe
introduces (e.g. checking for whether the Maybe is a Some or None at
certain points).
Attachment #8761258 - Flags: review?(nchen)
Comment on attachment 8761258 [details] [diff] [review]
avoid invalid copying of objects in PrefsHelper.h

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

Thanks!
Attachment #8761258 - Flags: review?(nchen) → review+
Actually, I wonder if we can add a move constructor to StringParam. Design-wise, I think it should be fine to have a move constructor in StringParam.
Flags: needinfo?(nchen)
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4f903cba94d
avoid invalid copying of objects in PrefsHelper.h; r=darchons
https://hg.mozilla.org/mozilla-central/rev/d4f903cba94d
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.