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

[Daniel Dunbar]

Ping?

On Sat, Dec 19, 2009 at 11:14 AM, Daniel Dunbar <daniel at zuster.org> wrote:
> 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
>>
>