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

[Ken Dyck]

On Tuesday, December 15, 2009 10:37 AM, I wrote:
> 
> 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.
> > > 
> > > [snipped]
> > 
> > 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?

[silence]

I'll take this silence as a sign that if there are any objections to my
revised patch, they are minor. Committed as 91683. Comments still
welcome.

-Ken