Bug 157458 – [Bindings] Add convert<>() template specializations for integer types

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Chris Dumez]

Add convert<>() template specializations for integer types and use them in the JS bindings.

[Chris Dumez]

Created attachment 278357 [details]
Patch

[Darin Adler]

Comment on attachment 278357 [details]
Patch

This looks good to me. I am concerned that our new JSDOMConvert.h things are just duplication of JSDOMBinding.h things. Eventually we don’t want so many different ways of doing the same thing.

[Chris Dumez]

Created attachment 278359 [details]
Patch

[Darin Adler]

Comment on attachment 278359 [details]
Patch

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

> Source/WebCore/bindings/js/JSCryptoAlgorithmDictionary.cpp:158
> -static std::unique_ptr<CryptoAlgorithmParameters> createAesKeyGenParams(ExecState* exec, JSValue value)
> +static std::unique_ptr<CryptoAlgorithmParameters> createAesKeyGenParams(ExecState* state, JSValue value)

Generally I have been trying to change the type from ExecState* to ExecState& at the same time I change the name from exec to state.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4239
> -    "long long" => "long long",
> +    "long long" => "int64_t",

I’m not completely sure I understand this change. Seems like if we did this we’d also want to use int32_t and uint32_t.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4245
> -    "unsigned long long" => "unsigned long long",
> +    "unsigned long long" => "uint64_t",

Same comment here.

It seems that on Unix builds we are building on platforms that use 64-bit for long, so uint64_t is the same thing as "unsigned long" rather than "unsigned long long". (As an aside, that means that we need to be careful to never use the type "long" or "unsigned long", assuming they are 32 bits.)

This breaks the binding for IDBFactory, which is using Optional<unsigned long long> in the C++ header, not Optional<uint64_t>. Both are 64-bit types, but they are distinct types.

I think we will probably need to either go all in on the native C types or all in on the sized ones. Because IDL syntax looks like native C types, I think that’s the direction I personally would slightly prefer. So we would use "unsigned char" and "signed char" and "short" rather than using "uint8_t" and "int8_t" and "int16_t". On the other hand, those type names are all longer and then don’t go well with the names of the integer conversion functions with numbers in their names. There are other arguments in both directions two.

Or we could continue to use a mix like this, but then we would have to move to only use int64_t and uint64_t rather than "long long" and "unsigned long long" in the code.

[Darin Adler]

Comment on attachment 278359 [details]
Patch

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

>> Source/WebCore/bindings/js/JSCryptoAlgorithmDictionary.cpp:158
>> -static std::unique_ptr<CryptoAlgorithmParameters> createAesKeyGenParams(ExecState* exec, JSValue value)
>> +static std::unique_ptr<CryptoAlgorithmParameters> createAesKeyGenParams(ExecState* state, JSValue value)
> 
> Generally I have been trying to change the type from ExecState* to ExecState& at the same time I change the name from exec to state.

Generally I have been trying to change the type from ExecState* to ExecState& at the same time I change the name from exec to state.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4239
>> -    "long long" => "long long",
>> +    "long long" => "int64_t",
> 
> I’m not completely sure I understand this change. Seems like if we did this we’d also want to use int32_t and uint32_t.

I’m not completely sure I understand this change. Seems like if we did this we’d also want to use int32_t and uint32_t.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4245
>> -    "unsigned long long" => "unsigned long long",
>> +    "unsigned long long" => "uint64_t",
> 
> Same comment here.
> 
> It seems that on Unix builds we are building on platforms that use 64-bit for long, so uint64_t is the same thing as "unsigned long" rather than "unsigned long long". (As an aside, that means that we need to be careful to never use the type "long" or "unsigned long", assuming they are 32 bits.)
> 
> This breaks the binding for IDBFactory, which is using Optional<unsigned long long> in the C++ header, not Optional<uint64_t>. Both are 64-bit types, but they are distinct types.
> 
> I think we will probably need to either go all in on the native C types or all in on the sized ones. Because IDL syntax looks like native C types, I think that’s the direction I personally would slightly prefer. So we would use "unsigned char" and "signed char" and "short" rather than using "uint8_t" and "int8_t" and "int16_t". On the other hand, those type names are all longer and then don’t go well with the names of the integer conversion functions with numbers in their names. There are other arguments in both directions two.
> 
> Or we could continue to use a mix like this, but then we would have to move to only use int64_t and uint64_t rather than "long long" and "unsigned long long" in the code.

Same comment here.

It seems that on Unix builds we are building on platforms that use 64-bit for long, so uint64_t is the same thing as "unsigned long" rather than "unsigned long long". (As an aside, that means that we need to be careful to never use the type "long" or "unsigned long", assuming they are 32 bits.)

This breaks the binding for IDBFactory, which is using Optional<unsigned long long> in the C++ header, not Optional<uint64_t>. Both are 64-bit types, but they are distinct types.

I think we will probably need to either go all in on the native C types or all in on the sized ones. Because IDL syntax looks like native C types, I think that’s the direction I personally would slightly prefer. So we would use "unsigned char" and "signed char" and "short" rather than using "uint8_t" and "int8_t" and "int16_t". On the other hand, those type names are all longer and then don’t go well with the names of the integer conversion functions with numbers in their names. There are other arguments in both directions two.

Or we could continue to use a mix like this, but then we would have to move to only use int64_t and uint64_t rather than "long long" and "unsigned long long" in the code.

[Chris Dumez]

Ok, I did not realize they were distinct types. I'll use the native C types more consistently then (including in the convert<>() specializations).

[Darin Adler]

(In reply to comment #6)
> Ok, I did not realize they were distinct types. I'll use the native C types
> more consistently then (including in the convert<>() specializations).

The thing that makes this tricky is that they are not exactly distinct types. The native C types are all real and distinct. The intxxx_t type are typedefs that map to one of the native C types.

[Chris Dumez]

(In reply to comment #6)
> Ok, I did not realize they were distinct types. I'll use the native C types
> more consistently then (including in the convert<>() specializations).

Actually, given the difference between platform of the native C types, I think I prefer to use the sized types in the bindings. The sized types are also shorted.

I think that the fact that we need to map unsigned long to unsigned, and long to int is a bit confusing otherwise. Web IDL clearly defines those types to be 32bit, so mapping to uint32_t / int32_t is a bit nicer IMHO.

[Chris Dumez]

Created attachment 278364 [details]
Patch

[Chris Dumez]

Comment on attachment 278364 [details]
Patch

Clearing flags on attachment: 278364

Committed r200556: <http://trac.webkit.org/changeset/200556>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.