435995 - Implement an nsIVariant object storage can use

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect)

Description

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Spun out of Bug 435994 for ease of review.

This is a templated class that implements nsIVariant.  Works wonderfully in tests so far, but not sure if reviewers will like it.

This can be useful for more than just the async api once it lands too.

Comment 1

[Shawn Wilsher :sdwilsh]

Comment on attachment 322710 [details] [diff] [review]
v1.0

I think this is ready for review.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Tests?  Usage?  I'm not actually sure what this does :)

Comment 3

[Shawn Wilsher :sdwilsh]

Tests up the dependency tree.  This is part of the async api (bug 429986) stuff but I'm trying to separate parts to make reviewing easier.

Comment 4

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

Addresses naming concerns.  Shaver wanted bsmedberg to review this.

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 326805 [details] [diff] [review]
v1.1

bsmedberg thinks dbaron should review this...

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Why me?  I've got a bunch of other stuff in my queue that does require my review.


What's the #include <utility> for?  How portable is it?

It seems like you could get rid of a good bit of code bloat by giving mozStorageVariant a non-templatized base class that has the methods that don't vary, and implementing them in a C++ file.

Could you describe what this class is for and how it's used?  Preferably in code comments?

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Also, it seems like the template parameter isn't use purely as a type; PRUint8 looks like it's code for "array", which seems at the very least a bit weird.

Comment 8

[Shawn Wilsher :sdwilsh]

(In reply to comment #6)
> Why me?  I've got a bunch of other stuff in my queue that does require my
> review.
shaver punted to bsmedberg, who punted to you on it.

> What's the #include <utility> for?  How portable is it?
I'm using std::pair.  As far as I know, it is portable.

> It seems like you could get rid of a good bit of code bloat by giving
> mozStorageVariant a non-templatized base class that has the methods that don't
> vary, and implementing them in a C++ file.
Yeah, that's actually a good idea.  Next version will do that.

> Could you describe what this class is for and how it's used?  Preferably in
> code comments?
Sure, that'll be in the next patch.

(In reply to comment #7)
> Also, it seems like the template parameter isn't use purely as a type; PRUint8
> looks like it's code for "array", which seems at the very least a bit weird.
Good point.  That will be changed to PRUint8[].

Comment 9

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

Addresses review comments.

Comment 10

[Rob Arnold [:robarnold]]

I don't like the name assign in variant_storage_traits<DataType>::assign(aData).
Maybe storage_form or convert or something like that would be better but assign doesn't actually assign anything so it's a bit misleading.

Comment 11

[Shawn Wilsher :sdwilsh]

Attachment: v1.3

Comment 12

[Rob Arnold [:robarnold]]

Comment on attachment 328676 [details] [diff] [review]
v1.3

be consistent in what you call the return type and parameters types for storage_conversion and the at* functions. To avoid accidentally creating overloaded members, you should probably use the typedef'd names.

Comment 13

[Shawn Wilsher :sdwilsh]

Attachment: v1.4

Addresses comments as discussed over im.

Comment 14

[Shawn Wilsher :sdwilsh]

Attachment: v1.4

ugh - bug numbers are too similar.  That was the wrong patch.

Comment 15

[Rob Arnold [:robarnold]]

Comment on attachment 328909 [details] [diff] [review]
v1.4

I like the new typedefs

Comment 16

[Shawn Wilsher :sdwilsh]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/3b714908eed1

Comment 17

[[e-mail]]

Comment on attachment 328909 [details] [diff] [review]
v1.4

>+    _result = NS_ConvertUTF16toUTF8(aValue);
CopyUTF16toUTF8(aValue, _result);

>+    nsTArray<PRUint8> data(aBlob.second);
>+    for (int i = 0; i < aBlob.second; i++)
>+      data.InsertElementAt(i, static_cast<const PRUint8 *>(aBlob.first)[i]);
data.AppendElements(static_cast<const PRUint8 *>(aBlob.first), aBlob.second);

>+    // Copy the array
>+    *_result = NS_Alloc(sizeof(PRUint8) * aData.Length());
>+    NS_ENSURE_TRUE(*_result, NS_ERROR_OUT_OF_MEMORY);
>+
>+    PRUint8 *result = static_cast<PRUint8 *>(*_result);
>+    for (PRUint32 i = 0; i < aData.Length(); i++)
>+      result[i] = aData[i];
*_result = nsMemory::Clone(aData.Elements(), aData.Length() * sizeof(PRUint8));