Clarifying style guide language around size_t?

[Peter Kasting]

Dear C++ arbiters,

The Google style guide has language regarding size_t (in http://google.github.io/styleguide/cppguide.html#Integer_Types ) that has repeatedly caused style disagreements, and I want to know if/how we should clarify this.

How the rules have always been explained to me, and what I enforced in my Readability reviews, is roughly:

• Don't use unsigned types merely to document that a value never goes negative; the value of that documentation does not exceed the downsides of unsigned types.
  
• However, if you are working with cases where size_t would be the language's natural type anyway -- that is, object lengths in memory, e.g. vector::size(), string::size(), sizeof expr, "length of data packet", etc. -- you are welcome to use size_t (and, in general, should do so, to reduce casting at e.g. STL boundaries, and to avoid potential truncation problems using int on a 64-bit machine).
  

However, I've gotten pushback from both directions in code reviews:

• Despite the phrase "you are welcome to use size_t", some authors have been reluctant to allow size_t at all.  (This view is distressingly prevalent in google3 itself, where I've had changes rejected because they added any use of size_t.  In Chromium, I've always been able to get these accepted, but sometimes needed more explanation.)
• In the change that triggered this email ( https://codereview.chromium.org/2255403002/ ), replacing size_t with int has been controversial because the concept in question is "the size of something" (in this case, the number of pixels on one side of a square image).  In other words, the author's contention is that if the meaning of the variable includes the concept of something's size, size_t should be allowed.  (Evan, please clarify if I misrepresented you.)

I have previously sought to have C-style clarify the style guide, but have not gotten anywhere.  I believe the style arbiters feel things are sufficiently clear, or else they are reluctant to change text that has been around a long time.

An alternative is that we add language to the Chromium style guide clarifying this, e.g. examples of good and bad usage.  This might be useful, but I'm wary of adding more content to that document, especially when it's not actually an exception to the Google style rules.  Perhaps modifying the "C++ Do's and Dont's" page would be better, but that page says it's non-binding on authors.

So the questions are:

• Is there rough consensus that the above interpretation of the rules is accurate?  Do we need to start a chromium-dev thread for this?
• Should we add language to clarify this somewhere?  If so where?

PK

[dan...@chromium.org]

The last c-style thread on this was last February: https://groups.google.com/a/google.com/d/topic/c-style/VUC5dtn59Zg/discussion It ended in roughly "there is no way to specify this rule, so do what feels right". I'd recommend reading it if you haven't seen that thread yet, if nothing else for entertainment.

I have felt in the past very happy with Chromium's style guide about size_t. It says "Use size_t for object and allocation sizes, object counts, array and pointer offsets, vector indices, and so on." which I always read as "Use size_t for numbers of bytes and for interacting with STL containers". This is a bit leaky, as the number of pixels is really the number of bytes (divided by 4).

FWIW, as I've seen it in compositor/gpu code we have used size_t to represent this because we end up doing memory allocations based on it, and converting between signed/unsigned is unpleasant. Any when not working with physical memory areas (ie width*height), dealing more with abstract/conceptual sizes or values representing a dimension along one axis, we've used ints. This distinction makes sense in my head, and is somewhat enforced/encouraged by our gfx::Size/Rect/Point types. What do you think?

In the review in question, https://codereview.chromium.org/2255403002/diff/20001/ui/gfx/paint_vector_icon.h says |dip_size| is the length of one edge of the icon, so an int would be the appropriate type according to my internal application of the rules. Logically this makes sense to me because if it wasn't a square and you had two dimensions, you'd want to use a gfx::Size which is two ints, and because an edge of a square is not logically the same as the number of bytes in memory.

Examples:

- Related to bytes but dimension along one axis: https://cs.chromium.org/chromium/src/cc/resources/resource_provider.h?rcl=0&l=106

- Number of bytes in a bitmap: https://cs.chromium.org/chromium/src/cc/resources/shared_bitmap.h?rcl=0&l=37

- Size in pixels via gfx::Size: https://cs.chromium.org/chromium/src/cc/tiles/picture_layer_tiling.h?rcl=0&l=118

• Should we add language to clarify this somewhere?  If so where?

I think if we add/change anything it should go with the rest of the paragraph about size_t in https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md.

 

PK

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFBOexku%3D%2BnVAccthV-BPfV7vaB%3DY_HpU5aS92JhkkQozQ%40mail.gmail.com.

[Brett Wilson]

On Mon, Aug 22, 2016 at 3:24 PM, Peter Kasting <pkas...@chromium.org> wrote:

Your description is always how I've understood this as well, and I've also been unhappy with the internal style guide's clarity here. It seems clear to me that using size_t for bitmap dimensions falls under the "unsigned types" restriction. This kind of thing is frequently used in layout computations which should be using signed types, so I thing the unsigned types are actively unhelpful in this case.

Brett

[Peter Kasting]

On Mon, Aug 22, 2016 at 3:49 PM, <dan...@chromium.org> wrote:

I have felt in the past very happy with Chromium's style guide about size_t. It says "Use size_t for object and allocation sizes, object counts, array and pointer offsets, vector indices, and so on." which I always read as "Use size_t for numbers of bytes and for interacting with STL containers".

I did not recall that we actually had language around size_t in our own style guide (mea culpa).  For everyone else, see https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#Types .

 

This is a bit leaky, as the number of pixels is really the number of bytes (divided by 4).

FWIW, as I've seen it in compositor/gpu code we have used size_t to represent this because we end up doing memory allocations based on it, and converting between signed/unsigned is unpleasant. Any when not working with physical memory areas (ie width*height), dealing more with abstract/conceptual sizes or values representing a dimension along one axis, we've used ints. This distinction makes sense in my head, and is somewhat enforced/encouraged by our gfx::Size/Rect/Point types. What do you think?

Yeah, I think that usage is fine.

Basically I would restate this as: once you know you're talking precisely about "object length in memory" -- e.g. where you're passing a pixel count to a value that's going to allocate memory for that many pixels -- you should be using size_t.  It's OK for this not to be 1:1 with bytes, just as vector<int>::size() returns a size_t that is a count of ints, not a count of bytes.

Before that, when you're working with arbitrary conceptual values, the units of "pixels" should not be size_t, because this is more a "countable thing" than a "thing with allocated memory backing it" and negative values actually make some sense.

I am handwaving.  I don't know how to state the distinction more clearly, but I agree with you that, in my head, it looks as if there's a line.

• Should we add language to clarify this somewhere?  If so where?

I think if we add/change anything it should go with the rest of the paragraph about size_t in https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md.

Yes, given that we have text there, I agree.

Given that we also have some examples of things that are size_ts, I'm not sure there's anything else needed.  Evan, do you think the text there is clear enough?  If not, please propose a change for discussion.

PK

[Evan Stade]

yea, it's pretty clear. I guess I made the same mistake you did of not double checking our own style guide.

-- Evan Stade