[PATCH] D48163: [clangd] UI for completion items that would trigger include insertion.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clangd/CodeComplete.cpp:301
+          } else if (!IncludePath->empty()) {
+            if (auto Edit = Includes.insert(*IncludePath)) {
+              I.detail += ((I.detail.empty() ? "" : "\n") +
----------------
This is fine for now, but can you add a FIXME to consider what to show if there's no include to be inserted, and for sema results? There's some value in showing the header consistently I think.


================
Comment at: clangd/CodeComplete.cpp:302
+            if (auto Edit = Includes.insert(*IncludePath)) {
+              I.detail += ((I.detail.empty() ? "" : "\n") +
+                           StringRef(*IncludePath).trim('"'))
----------------
can we add the \n for now even if detail is empty?
That way the editors that "inline" the detail into the same line as the label won't show it (if we patch YCM to truncate like VSCode does).
The inconsistency (sometimes showing a return type and sometimes a header) seems surprising.


================
Comment at: clangd/CodeComplete.cpp:313
+    I.label =
+        (InsertingInclude ? Opts.IncludeInsertionIndicator : " ") + I.label;
     I.scoreInfo = Scores;
----------------
string(IncludeInsertionIndicator.size(), ' ')?


================
Comment at: clangd/CodeComplete.cpp:334
         Opts.EnableSnippets ? (Name + "(${0})").str() : Name.str();
-    First.label = (Name + "(…)").str();
+    // Keep the first character of the original label which is an visual
+    // indicator for include insertion or an indentation.
----------------
again, need to account for the length of the indicator


================
Comment at: clangd/CodeComplete.h:63
 
-  // Populated internally by clangd, do not set.
+  /// Populated internally by clangd, do not set.
   /// If `Index` is set, it is used to augment the code completion
----------------
if you want this to be part of the doc comment, move it to the bottom?

I think the intent was that this comment apply to all following members. So keeping the line as `//` and adding the new member above it would also make sense


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48163