[PATCH] Implemented llvm::sys::locale::columnWidth and isPrint for the case of generic UTF8-capable terminal.

Manuel Klimek klimek at google.com
Thu Aug 8 10:57:31 PDT 2013


  Sorry for jumping in after-the-fact. Consider those ideas for follow-up patches.


================
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
----------------
To me this whole function looks like it would be better put into a unit test.

================
Comment at: llvm/trunk/include/llvm/Support/UnicodeCharRanges.h:22
@@ +21,3 @@
+
+struct UnicodeCharRange {
+  uint32_t Lower;
----------------
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.


================
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));
----------------
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.


http://llvm-reviews.chandlerc.com/D1253

COMMIT
  http://llvm-reviews.chandlerc.com/rL187837



More information about the llvm-commits mailing list