You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2022/08/10 19:08:51 UTC

[GitHub] [tvm] gbonik commented on a diff in pull request #12344: [TVMScript] Text underlining in DocPrinter based on Doc's source_paths

gbonik commented on code in PR #12344:
URL: https://github.com/apache/tvm/pull/12344#discussion_r942807225


##########
src/script/printer/base_doc_printer.cc:
##########
@@ -23,19 +23,256 @@ namespace tvm {
 namespace script {
 namespace printer {
 
-DocPrinter::DocPrinter(int indent_spaces) : indent_spaces_(indent_spaces) {}
+namespace {
 
-void DocPrinter::Append(const Doc& doc) { PrintDoc(doc); }
+void SortAndMergeSpans(std::vector<ByteSpan>* spans) {
+  if (spans->empty()) {
+    return;
+  }
+  std::sort(spans->begin(), spans->end());
+  auto last = spans->begin();
+  for (auto cur = spans->begin() + 1; cur != spans->end(); ++cur) {
+    if (cur->first > last->second) {
+      *++last = *cur;
+    } else if (cur->second > last->second) {
+      last->second = cur->second;
+    }
+  }
+  spans->erase(++last, spans->end());
+}
+
+size_t GetTextWidth(const std::string& text, const ByteSpan& span) {
+  // FIXME: this only works for ASCII characters.
+  // To do this "correctly", we need to parse UTF-8 into codepoints
+  // and call wcwidth() or equivalent for every codepoint.
+  size_t ret = 0;
+  for (size_t i = span.first; i != span.second; ++i) {
+    if (isprint(text[i])) {
+      ret += 1;
+    }
+  }
+  return ret;
+}
+
+size_t MoveBack(size_t pos, size_t distance) { return distance > pos ? 0 : pos - distance; }
+
+size_t MoveForward(size_t pos, size_t distance, size_t max) {
+  return distance > max - pos ? max : pos + distance;
+}
+
+size_t GetLineIndex(size_t byte_pos, const std::vector<size_t>& line_starts) {
+  auto it = std::upper_bound(line_starts.begin(), line_starts.end(), byte_pos);
+  return (it - line_starts.begin()) - 1;
+}
+
+using UnderlineIter = typename std::vector<ByteSpan>::const_iterator;
+
+ByteSpan PopNextUnderline(UnderlineIter* next_underline, UnderlineIter end_underline) {
+  if (*next_underline == end_underline) {
+    return {std::numeric_limits<size_t>::max(), std::numeric_limits<size_t>::max()};
+  } else {
+    return *(*next_underline)++;
+  }
+}
+
+void PrintChunk(const std::pair<size_t, size_t>& lines,
+                const std::pair<UnderlineIter, UnderlineIter>& underlines, const std::string& text,
+                const std::vector<size_t>& line_starts, const DocPrinterOptions& options,
+                size_t line_number_width, std::string* out) {
+  UnderlineIter next_underline = underlines.first;
+  ByteSpan current_underline = PopNextUnderline(&next_underline, underlines.second);
+
+  for (size_t line_idx = lines.first; line_idx < lines.second; ++line_idx) {
+    if (options.print_line_numbers) {
+      std::string line_num_str = std::to_string(line_idx + 1);
+      line_num_str.push_back(' ');
+      for (size_t i = line_num_str.size(); i < line_number_width; ++i) {
+        out->push_back(' ');
+      }
+      *out += line_num_str;
+    }
+
+    size_t line_start = line_starts.at(line_idx);
+    size_t line_end =
+        line_idx + 1 == line_starts.size() ? text.size() : line_starts.at(line_idx + 1);
+    out->append(text.begin() + line_start, text.begin() + line_end);
+
+    bool printed_underline = false;
+    size_t line_pos = line_start;
+    bool printed_extra_caret = 0;
+    while (current_underline.first < line_end) {
+      if (!printed_underline) {
+        *out += std::string(line_number_width, ' ');
+        printed_underline = true;
+      }
+
+      size_t underline_end_for_line = std::min(line_end, current_underline.second);
+      size_t num_spaces = GetTextWidth(text, {line_pos, current_underline.first});
+      if (num_spaces > 0 && printed_extra_caret) {
+        num_spaces -= 1;
+        printed_extra_caret = false;
+      }
+      *out += std::string(num_spaces, ' ');

Review Comment:
   I'm not sure, I could see arguments either way.
   ```
   for i in T.serial(10):
   ^^^^^^^^^^^^^^^^^^^^^^
       a[i] = 5
   ^^^^^^^^^^^^
   ```
   
   ```    
   for i in T.serial(10):
   ^^^^^^^^^^^^^^^^^^^^^^
       a[i] = 5
       ^^^^^^^^
   ```
   I'd say that the first option can provide a better sense of "continuity". make it clear that this is one chunk of text being highlighted, as opposed to two different chunks. Also it seems to simplify the implementation :) 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org