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

[Chris Lattner]

On Dec 18, 2009, at 5:39 AM, Ken Dyck wrote:

> Author: kjdyck
> Date: Fri Dec 18 07:39:46 2009
> New Revision: 91683
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=91683&view=rev
> Log:
> Initial implementation of CharUnits, an opaque value class for representing
> sizes, offsets, and alignments in character units.

This looks nice to me Ken, some minor comments:

> +++ 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.

> +#include <stdint.h>

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

> +#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. 

Otherwise, great addition!

-Chris