894012 - convert expungedBytes to 64bit

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Wayne Mery (:wsmwk)]

convert expungedBytes to 64bit

Comment 1

[:aceman]

This is a spinoff from bug 789679 comment 77 and needs to be done on top of patch "make GetSizeOnDisk 64 bit":

 :aceman 2013-03-20 09:15:27 CET

If the changes here are deemed OK, I can also convert expungedBytes to 64bit as somebody may need to clear whole >4GB folder and it may fail with 32bit expungedBytes variable.

Comment 2

[:aceman]

Attachment: patch

There is an open question whether we want the property to be int64 or uint64 in FolderInfo. folderSize was uint64 even when in nsIMsgFolder it was uint32. Now we moved it to int64 in nsIMsgFolder. Should we change it to int64 in FolderInfo so that they match? Should we then make expungedBytes the same?

Comment 3

[Kent James (:rkent)]

(In reply to :aceman from comment #2)
> Created attachment 8532736 [details] [diff] [review]
> patch
> 
> There is an open question whether we want the property to be int64 or uint64
> in FolderInfo. folderSize was uint64 even when in nsIMsgFolder it was
> uint32. Now we moved it to int64 in nsIMsgFolder. Should we change it to
> int64 in FolderInfo so that they match? Should we then make expungedBytes
> the same?

I found myself asking the same question as I reviewed the patch, before I even saw this comment. I even researched who set UInt64 for folderSize (bienvenu), as I was surprised it was that instead of Int64.

My sense is that we should convert to int64 in FolderInfo so that nsIDBFolderInfo has:

attribute long long expungedBytes;

Comment 4

[:aceman]

Should I do folderSize to int64_t too?

Comment 5

[Kent James (:rkent)]

I suppose that the correct issue is that folderSize should be in a separate patch. But the reality is probably that we would move faster if you just included it here. I don't mind if Neil doesn't.

Comment 6

[:aceman]

Attachment: patch v2

OK, made expungedBytes int64_t.

Comment 7

[Kent James (:rkent)]

Comment on attachment 8536102 [details] [diff] [review]
patch v2

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

Looks good, r+=me (but remove the question on SetUint64PropertyWithToken)

::: mailnews/db/msgdb/src/nsDBFolderInfo.cpp
@@ +567,5 @@
>  {
>    m_expungedBytes = expungedBytes;
> +  // ACE: not sure whether to create SetUint64PropertyWithToken for this
> +  // or use SetUint64Property as folderSize does.
> +  return SetUint64PropertyWithToken(m_expungedBytesColumnToken, m_expungedBytes);

If we bother to cache the token, then we should use it, so SetUint64PropertyWithToken is correct.

Comment 8

[:aceman]

Attachment: patch v2.1

Thanks.

Comment 9

[Kent James (:rkent)]

This is another critical bug that I have reviewed, and has now been on hold for a month waiting for a second review. Is there any reason that you really need for Neil to look at it? Yes there are risk issues - but it is also risky to leave bugs like this to the last minute before they are landed.

Comment 10

[:aceman]

This one changes an interface so had to have a superreview in the old days :) So I thought at least another review couldn't hurt. But if you think so, you can surely check it in.

Comment 11

[:aceman]

Neil, do you plan to look at this ever? :)

Comment 12

[[e-mail]]

Comment on attachment 8536821 [details] [diff] [review]
patch v2.1

Sorry for the delay. As well as the normal queue, I had a big server upgrade before Christmas and a big server failure after New Year to deal with. (And the week in between was mostly spent watching TV rather than doing reviews.)

>   GetInt32PropertyWithToken(m_folderDateColumnToken, (int32_t &) m_folderDate);
>   GetInt32PropertyWithToken(m_imapUidValidityColumnToken, m_ImapUidValidity, kUidUnknown);
>   GetInt32PropertyWithToken(m_expiredMarkColumnToken, (int32_t &) m_expiredMark);
>-  GetInt32PropertyWithToken(m_expungedBytesColumnToken, (int32_t &) m_expungedBytes);
>+  GetUint64PropertyWithToken(m_expungedBytesColumnToken, (uint64_t *) &m_expungedBytes);
>   GetInt32PropertyWithToken(m_highWaterMessageKeyColumnToken, (int32_t &) m_highWaterMessageKey);
Ideally you would create GetInt64PropertyWithToken(mdb_token, int64_t&) but I suppose you can do that as part of the conversion of the folder size to int64_t. Also at some point you could switch these to use GetUint32PropertyWithToken.

Comment 13

[:aceman]

(In reply to [e-mail] from comment #12)
> >   GetInt32PropertyWithToken(m_folderDateColumnToken, (int32_t &) m_folderDate);
> >   GetInt32PropertyWithToken(m_imapUidValidityColumnToken, m_ImapUidValidity, kUidUnknown);
> >   GetInt32PropertyWithToken(m_expiredMarkColumnToken, (int32_t &) m_expiredMark);
> >-  GetInt32PropertyWithToken(m_expungedBytesColumnToken, (int32_t &) m_expungedBytes);
> >+  GetUint64PropertyWithToken(m_expungedBytesColumnToken, (uint64_t *) &m_expungedBytes);
> >   GetInt32PropertyWithToken(m_highWaterMessageKeyColumnToken, (int32_t &) m_highWaterMessageKey);
> Ideally you would create GetInt64PropertyWithToken(mdb_token, int64_t&) but
> I suppose you can do that as part of the conversion of the folder size to
> int64_t. Also at some point you could switch these to use
> GetUint32PropertyWithToken.

Yes, I have marked uint64_t in folderSize for later work. I can file the bug for that.
+  uint64_t  m_folderSize;    // TODO: In nsIMsgFolder folder size is int64_t.

But what do you mean with switching to GetUint32PropertyWithToken ? Do you mean the GetInt32PropertyWithToken calls? Why should those be Uint, when 64 bit version are going to Int ?

Thanks for the review :)

Comment 14

[[e-mail]]

(In reply to aceman from comment #13)
> But what do you mean with switching to GetUint32PropertyWithToken ? Do you
> mean the GetInt32PropertyWithToken calls? Why should those be Uint, when 64
> bit version are going to Int ?

Only because those casts are ugly and using the available GetUint32PropertyWithToken hides the ugliness a little.

Comment 15

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Pushed to comm-central changeset 332703d2fcdf