[cfe-commits] [Review] CharUnits for ASTContext::getTypeSizeInBytes()

[Ken Dyck]

On Monday, December 14, 2009 9:55 PM, John McCall wrote:
> 
> On Dec 14, 2009, at 12:55 PM, Ken Dyck wrote:
> 
> > As discussed earlier [1] (apologies for the delay), the 
> attached patch 
> > introduces CharUnits, an opaque value class for quantities with 
> > character units.
> > 
> > It uses an int64_t to represent the quantity, has a named 
> constructor, 
> > a single explicit conversion to a int64_t, and basic arithmetic, 
> > comparison, and relational operators. It is intended as a 
> return type 
> > for ASTContext::getTypeSizeInBytes() and other methods that return 
> > sizes, offsets and alignments in character units.
> > 
> > Comments welcome.
> 
> Looks great, thanks!  It'd be nice if you could completely 
> eliminate the abstraction overhead, which should be 
> straightforward:  just (1) use CharUnits() instead of Zero 
> and (2) define that lonesome operator* in the header (with 
> 'inline' to satisfy the linker).

Okay. I've moved all the method definitions to the header file and
removed the .cpp file.

I'd like to keep the concept of "Zero" in the class because there are
many places in the code where sizes are checked for zero and offsets are
initialized to zero (and I find "if (sz == CharUnits())", for example,
to be somewhat obscure). So instead of static class variable, I've
changed it to a named constructor in the attached patch. It is
implemented in the class body so a smart compiler can inline it. 

I've also added another similar constructor named One, since it is
another frequently used literal value in size-related code.

Finally, I added a few new predicates: isZero(), isOne(), isPositive(),
isNegative()  because these tests appear fairly frequently in
size-related code.

What do you think?

-Ken
-------------- next part --------------
A non-text attachment was scrubbed...
Name: CharUnits-v2.r91240.patch
Type: application/octet-stream
Size: 4225 bytes
Desc: CharUnits-v2.r91240.patch
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20091215/f1b5ba5f/attachment.obj>