Bug 118228 - Build fix: use of long long in CoreIPC::ArgumentEncoder and CoreIPC::ArgumentDecoder
Summary: Build fix: use of long long in CoreIPC::ArgumentEncoder and CoreIPC::Argument...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kwang Yul Seo
URL:
Keywords:
Depends on:
Blocks: 108832
  Show dependency treegraph
 
Reported: 2013-06-30 23:43 PDT by Kwang Yul Seo
Modified: 2013-07-25 15:35 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.28 KB, patch)
2013-06-30 23:47 PDT, Kwang Yul Seo
no flags Details | Formatted Diff | Diff
Patch (3.10 KB, patch)
2013-07-01 10:27 PDT, Kwang Yul Seo
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kwang Yul Seo 2013-06-30 23:43:32 PDT
While porting NetworkProcess to Gtk port, I encountered the following compile errors:

In file included from ../../Source/WebKit2/Platform/CoreIPC/ArgumentDecoder.h:29:0,
                 from ../../Source/WebKit2/Platform/CoreIPC/ArgumentCoders.h:29,
                 from ../../Source/WebKit2/Shared/FileAPI/BlobRegistrationData.cpp:31:
../../Source/WebKit2/Platform/CoreIPC/ArgumentCoder.h: In instantiation of `static bool CoreIPC::ArgumentCoder<T>::decode(CoreIPC::ArgumentDecoder&, T&) [with T = long long int]':
../../Source/WebKit2/Platform/CoreIPC/ArgumentDecoder.h:91:49:   required from `bool CoreIPC::ArgumentDecoder::decode(T&) [with T = long long int]'
../../Source/WebKit2/Shared/FileAPI/BlobRegistrationData.cpp:142:39:   required from here
../../Source/WebKit2/Platform/CoreIPC/ArgumentCoder.h:44:36: error: `decode' is not a member of `long long int'
../../Source/WebKit2/Platform/CoreIPC/ArgumentCoder.h: In instantiation of `static void CoreIPC::ArgumentCoder<T>::encode(CoreIPC::ArgumentEncoder&, const T&) [with T = long long int]':
../../Source/WebKit2/Platform/CoreIPC/ArgumentEncoder.h:57:9:   required from `void CoreIPC::ArgumentEncoder::encode(const T&) [with T = long long int]'
../../Source/WebKit2/Platform/CoreIPC/ArgumentEncoder.h:62:9:   required from `CoreIPC::ArgumentEncoder& CoreIPC::ArgumentEncoder::operator<<(const T&) [with T = long long int; CoreIPC::ArgumentEncoder = CoreIPC::ArgumentEncoder]'
../../Source/WebKit2/Shared/FileAPI/BlobRegistrationData.cpp:91:29:   required from here
../../Source/WebKit2/Platform/CoreIPC/ArgumentCoder.h:39:9: error: request for member `encode' in `t', which is of non-class type `const long long int'
../../Source/WebKit2/Platform/CoreIPC/ArgumentCoder.h: In static member function `static bool CoreIPC::ArgumentCoder<T>::decode(CoreIPC::ArgumentDecoder&, T&) [with T = long long int]':
../../Source/WebKit2/Platform/CoreIPC/ArgumentCoder.h:45:5: warning: control reaches end of non-void function [-Wreturn-type]
ICECC[1565] 15:39:58: Compiled on 192.168.100.227

int64_t fixes build.
Comment 1 Kwang Yul Seo 2013-06-30 23:47:26 PDT
Created attachment 205785 [details]
Patch
Comment 2 Anders Carlsson 2013-07-01 07:36:41 PDT
Comment on attachment 205785 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=205785&action=review

> Source/WebCore/platform/network/BlobData.h:149
> -    long long offset;
> -    long long length;
> +    int64_t offset;
> +    int64_t length;
>      double expectedModificationTime;

I don't think we should change code in WebCore because of IPC requirements.
Comment 3 Kwang Yul Seo 2013-07-01 09:15:15 PDT
(In reply to comment #2)
> I don't think we should change code in WebCore because of IPC requirements.

Okay. Then do you want to add long long type to ArgumentEncoder::encode and ArgumentDecoder::decode?  The problem with this approach is that int64_t and long long are the same type on some platforms. 

For example, the current Mac port does not have this problem because they are the same type. So adding ArgumentEncoder::encode(long long) might conflict with ArgumentEncoder::encode(int64_t) on these platforms. (can't be overloaded)

So unless if you have a strong requirement for using long long over int64_t, I suggest to use int64. I understand long long is preferred in some cases because long long is at least 64 bit wide while int64_t is exactly 64 bit wide. But I think 64 bit is wide enough for offset and length here.

Or do you have any other suggestion?
Comment 4 Alexey Proskuryakov 2013-07-01 09:19:33 PDT
> Okay. Then do you want to add long long type to ArgumentEncoder::encode and ArgumentDecoder::decode?

Definitely not, IPC types should be all fixed size.

Personally, I'd be OK with the current patch, but the way to address the comment Anders made is to add explicit casts (with value checks) in BlobRegistrationData encode and decode functions.
Comment 5 Anders Carlsson 2013-07-01 09:54:53 PDT
(In reply to comment #4)

> Personally, I'd be OK with the current patch, but the way to address the comment Anders made is to add explicit casts (with value checks) in BlobRegistrationData encode and decode functions.

Explicit casts sounds good.
Comment 6 Kwang Yul Seo 2013-07-01 10:27:58 PDT
Created attachment 205820 [details]
Patch
Comment 7 Kwang Yul Seo 2013-07-01 10:28:43 PDT
(In reply to comment #5)
> Explicit casts sounds good.

Done. Thanks for the suggestion!
Comment 8 Kwang Yul Seo 2013-07-24 17:08:52 PDT
The patch is ready for review. Alexey and Anders, would you review the patch again?
Comment 9 Kwang Yul Seo 2013-07-25 15:35:25 PDT
Committed r153352: <http://trac.webkit.org/changeset/153352>