[cfe-commits] r91683 - /cfe/trunk/include/clang/AST/CharUnits.h

[Ken Dyck]

On Friday, December 18, 2009 12:56 PM, Chris Lattner wrote:
> 
> On Dec 18, 2009, at 5:39 AM, Ken Dyck wrote:
> 
> > +++ cfe/trunk/include/clang/AST/CharUnits.h Fri Dec 18 07:39:46 2009
> > +#ifndef LLVM_CLANG_AST_CHARUNITS_H
> > +#define LLVM_CLANG_AST_CHARUNITS_H
> > +
> > +#include "llvm/ADT/StringExtras.h"
> 
> Please move the toString() method out of line, to avoid this #include.

Okay.

> > +#include <stdint.h>
> 
> I don't think that stdint.h is fully portable, please use 
> llvm/Support/DataTypes.h instead to get int64_t.

It looks like Daniel has beat me to this one. Apologies for not fixing
this sooner (blame an extended end-of-year holiday lunch :).

> > +#include <string>
> > +
> > +namespace clang {
> > +  // An opaque type for sizes expressed in character units
> > +  class CharUnits {
> 
> Please add a doxygen comment (///) that explains the purpose 
> of this class an explains why charunits are not bytes :).  
> Also, in this comment and others, please end sentences with a period. 

Okay.

I hope to commit these changes as soon as I've built and verified they
don't break anything.

-Ken