Ping? On Sat, Dec 19, 2009 at 11:14 AM, Daniel Dunbar <[email protected]> wrote: > On Fri, Dec 18, 2009 at 9:56 AM, Chris Lattner <[email protected]> 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 >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
