[PATCH] D75687: [clangd] Only minimally escape text when rendering to markdown.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Sorry for the slow turnaround, bit off more than I could chew on a few fronts :-(



================
Comment at: clang-tools-extra/clangd/FormattedString.cpp:69
+    return false;
+  if (Contents.front() == '!' || Contents.front() == '?' ||
+      Contents.front() == '/')
----------------
kadircet wrote:
> what is the `?` for ?
`<?foo...` is a processing instruction, which is valid inline HTML (and therefore needs escaping)


================
Comment at: clang-tools-extra/clangd/FormattedString.cpp:132
+      return false;
+    llvm::StringRef Rest = After.ltrim(C);
+    return Rest.empty() || Rest.startswith(" ");
----------------
kadircet wrote:
> nit: After.ltrim('#')
I'm trying to avoid repeating the known values of C as there's various fallthrough/copied cases and it seems more fragile.


================
Comment at: clang-tools-extra/clangd/FormattedString.cpp:164
+  case '<': // HTML tag (or autolink, which we choose not to escape)
+    return looksLikeTag(After);
+  case '>': // Quote marker. Needs escaping at start of line.
----------------
kadircet wrote:
> is this really worth all the trouble ?
Simplified it a bit. I think it's worth not escaping `<` when not matched with `>`. Sadly the rule can't be quite that simple because multiple chunks can form a tag.


================
Comment at: clang-tools-extra/clangd/FormattedString.cpp:166
+  case '>': // Quote marker. Needs escaping at start of line.
+    return StartsLine && Before.empty();
+  case '&': { // HTML entity reference
----------------
kadircet wrote:
> I believe it is also allowed to have whitespaces(less than 4?) before the `>` to be interpreted as a quote marker.
Yes, but a chunk never begins with whitespace (see assert at top of function)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75687/new/

https://reviews.llvm.org/D75687