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

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Nathan Froyd [:froydnj]]

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?

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: avoid invalid copying of objects in PrefsHelper.h

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).

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8761258 [details] [diff] [review]
avoid invalid copying of objects in PrefsHelper.h

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

Thanks!

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

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.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4f903cba94d
avoid invalid copying of objects in PrefsHelper.h; r=darchons

Comment 5

[Carsten Book [:Tomcat]]

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