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

[Daniel Dunbar]

On Fri, Dec 18, 2009 at 9:56 AM, Chris Lattner <clattner at apple.com> wrote:
>
> 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.

Actually, can you just remove the toString method()? Clients can do
this themselves, its not a common operation, and I would like the
#include <string> removed as well.

 - Daniel

>> +#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
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>