Add convert<>() template specializations for integer types and use them in the JS bindings.
Created attachment 278357 [details] Patch
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.
Created attachment 278359 [details] Patch
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.
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.
Ok, I did not realize they were distinct types. I'll use the native C types more consistently then (including in the convert<>() specializations).
(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.
(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.
Created attachment 278364 [details] Patch
Comment on attachment 278364 [details] Patch Clearing flags on attachment: 278364 Committed r200556: <http://trac.webkit.org/changeset/200556>
All reviewed patches have been landed. Closing bug.