[PATCH] Implemented llvm::sys::locale::columnWidth and isPrint for the case of generic UTF8-capable terminal.
Jordan Rose
jordan_rose at apple.com
Thu Aug 8 11:01:08 PDT 2013
================
Comment at: llvm/trunk/include/llvm/Support/UnicodeCharRanges.h:78
@@ +77,3 @@
+LLVM_READONLY static inline bool isCharInSet(uint32_t C,
+ UnicodeCharSet CharSet) {
+ assert(isValidCharSet(CharSet));
----------------
Manuel Klimek wrote:
> If we define operator<(uint32_t, UnicodeCharRange) and
> operator<(UnicodeCharRange, uint32_t), this becomes
> std::binary_search(CharSet.begin(), CharSet.end(), C);
> I pretty strongly vote against adding an implementation of binary search.
Interesting. Feels weird to me, but I guess there's actually a reasonable interpretation for both of those even outside this context.
================
Comment at: llvm/trunk/include/llvm/Support/UnicodeCharRanges.h:22
@@ +21,3 @@
+
+struct UnicodeCharRange {
+ uint32_t Lower;
----------------
Manuel Klimek wrote:
> I think it makes sense to put a comment here on the closedness of the ranges.
> Defining a "is-in" on the class would also basically be self-documenting.
>
I like the idea of using a method, but the actual implementation of the search wouldn't benefit from it. Your other comment may make this unnecessary, though.
================
Comment at: llvm/trunk/include/llvm/Support/UnicodeCharRanges.h:30
@@ +29,3 @@
+/// [min, max], and if the ranges themselves are ordered and non-overlapping.
+static inline bool isValidCharSet(UnicodeCharSet CharSet) {
+#ifndef NDEBUG
----------------
Manuel Klimek wrote:
> To me this whole function looks like it would be better put into a unit test.
I got this feedback when I implemented it the first time; the reason I was worried about that is because I anticipating someone adding a new set and forgetting to add a test. This way it's forced just by calling `isCharInSet`.
http://llvm-reviews.chandlerc.com/D1253
COMMIT
http://llvm-reviews.chandlerc.com/rL187837
More information about the llvm-commits
mailing list