[PSA] C++ integer types != WebIDL integer types

[Dave Tapuska]

I've been occasionally working on a patch that turns on narrowing detection in blink and I've found a number of issues specifically around implementation of IDL interfaces. In fact the test code for the bindings layer is also problematic so I've decided to write this message because it is one of these subtle things.

WebIDL defines

long/unsigned long =  32 bits

long long/unsigned long long = 64 bits

Ok as long as I always remember to look at the spec I can keep these clear in my mind.

C++ Defines

long/unsigned long = 32 or 64 bits (depends on Linux or Windows)

long long/unsigned long long = 64 bits

size_t depends on the size of a ptr, 64 bits for 64 bit builds, 32 bits for 32 bit builds.

Yes long on Linux on a 64 bit platform is 8 bytes yet on Windows on the same CPU architecture it is 4 bytes. Intel has some information here.

Please, please stick to using stdint types; eg. uint64_t/int64_t/int64_t/uint64_t and avoid using size_t on interfaces implemented to the IDL.

Blink also tends to use a lot of unsigned or int and I urge you to start using the stdint types which are relatively easy to "understand how wide" is this field.

Some of this will be automatically caught when I get around to fully enabling the warnings but I thought it was useful to communicate some guidance.

dave.

[Kentaro Hara]

+1 to using stdint types in the Blink code base as much as possible.

I vaguely remember that we discussed this a couple of years ago for the exact reason you pointed out; i.e., given the subtle difference of 'long / long long' between Web IDL and C++, we should stick to using stdint types. The conclusion at that time was that we should do this but we didn't actually make that change just because it was low priority... So +1 to making that change now :)

If there's no objection on this thread, would you add the guideline to blink/renderer/README.md? :)

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAHgVhZWu-ae%3DZFQ95GJD6mzGsTGL5J72_RubWpJTV3Bq_vH15w%40mail.gmail.com.

--

Kentaro Hara, Tokyo, Japan

[Peter Kasting]

On Sat, Jul 21, 2018 at 1:23 AM Kentaro Hara <har...@chromium.org> wrote:

+1 to using stdint types in the Blink code base as much as possible.

Can you be clear here?  "As much as possible" sounds far broader than "where there's a boundary with IDL types".

The Google style guide already bans the core C integer types other than int in http://google.github.io/styleguide/cppguide.html#Integer_Types .  However, using int32_t blindly everywhere in Blink would violate the guideline about "use plain old int for such things" on common uses like loop counters.

Chromium already has guidance in https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#types noting that people should use explicitly-sized types from stdint.h at network or process boundaries or where the exact size matters.  Perhaps the right change is to add something about IDL.

PK

[Kentaro Hara]

Thanks Peter for pointing this out!

Can you be clear here?  "As much as possible" sounds far broader than "where there's a boundary with IDL types".

Yeah, you're right. "Where there's a boundary with IDL types" sounds reasonable.

However, Blink might have more use cases of stdint types though. Recently we decided to use int32_t for the type of WTF::Vector::length_ (discussion thread, crbug.com/652586). This means that caller sites of WTF::Vector are expected to index vectors with int32_t rather than int or size_t. Are we probably doing a wrong thing here?

[Dave Tapuska]

A lot of the index iterators currently use size_t. They will move to be uint32_t with my changes for wtf::vector. I generally don't see a plain int used much these days. 

Dave

[Peter Kasting]

On Sat, Jul 21, 2018 at 1:09 PM Kentaro Hara <har...@chromium.org> wrote:

Can you be clear here?  "As much as possible" sounds far broader than "where there's a boundary with IDL types".

Yeah, you're right. "Where there's a boundary with IDL types" sounds reasonable.

However, Blink might have more use cases of stdint types though. Recently we decided to use int32_t for the type of WTF::Vector::length_ (discussion thread, crbug.com/652586). This means that caller sites of WTF::Vector are expected to index vectors with int32_t rather than int or size_t. Are we probably doing a wrong thing here?

Yes.  You should be using size_t, not int32_t, for that, per both the Google and Chromium style guides.

Please don't move things that are correctly using size_t away from it; fix the places that are incorrectly not using size_t to use it.  I'm pretty passionate about this specific issue due to having spent several years of time trying to fix type usage, and especially the failure to use size_t where it's justified, in Chromium and WebRTC.

PK.