[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