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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/CodeComplete.cpp:187
+  // Whether or not this candidate is in a completion where index is allowed.
+  // This can affect how items are built (e.g. indent label to align with visual
+  // indicator in index results).
----------------
Hmm, I'm not sure this should be conditional.
Especially in editors that render [icon]label, inconsistently having a space or not for no obvious reason seems disorienting to users.

And it adds some complexity (especially in conjunction with bundling...)


================
Comment at: clangd/CodeComplete.cpp:266
+          } else {
+            InsertingInclude = static_cast<bool>(HeaderAndEdit->second);
+            if (InsertingInclude) {
----------------
nit: .hasValue() would be clearer than static_cast


================
Comment at: clangd/CodeComplete.cpp:278
+    if (AllowIndexCompletion)
+      I.label = (InsertingInclude ? "+" : " ") + I.label;
     I.scoreInfo = Scores;
----------------
ioeric wrote:
> sammccall wrote:
> > I think we should avoid tokens that occur commonly in C++ (possibly with the exception of # which is used for this purpose. We should also avoid doublewide chars because of alignment (even assuming a monospace font is risky...)
> > 
> > Ideas:
> > - something like "external link" icon on wikipedia - but no such thing in unicode :-(🔗(U+1F517 link) looks truly terrible in at least some fonts
> > - variant on plus like ⊕ (U+2295 circled plus) - can't find one I like a lot
> > - some kind of arrow like ☇ (U+2607 lightning) or ⇱ (U+21F1 north west arrow to corner) or ⮣ (upwards triangle-headed arrow with long head rightwards)
> > - variant on hash like ﹟(U+FE5F  small number sign)
> > - something unrelated but "special" like ※ (U+203B reference mark)
> > 
> > I'm not really happy with any of these :-(
> Thanks for the suggestions! 
> 
> After playing around with different options, we decided to go with `•` which is simple and also aligns well in vim and vscode. We tried to avoid symbols that are meaningful/complicated because they tend to add noise when users are used to the existence of the icon, and they appear less elegant when there are a whole list of indicators in the completion results.
this is going to be slightly tricky with bundling, where we take the CompletionItem for an arbitrary item, and recompute the label as Name + "(...)".

Note that if we're unconditionally adding the padding character, we can just grab it out of the old CompletionItem.label :-) Yes it's a hack, but otherwise we need to pass this information around or factor quite differently.


================
Comment at: clangd/Headers.cpp:75
 /// path is not shortened.
-llvm::Expected<std::string> calculateIncludePath(
+llvm::Expected<std::pair<std::string, bool>> calculateIncludePath(
     PathRef File, StringRef BuildDir, HeaderSearch &HeaderSearchInfo,
----------------
we don't need Expected here anymore?


================
Comment at: clangd/Headers.h:63
 /// same as DeclaringHeader but must be provided.
-//  \return A quoted "path" or <path>. This returns an empty string if:
+/// \return A quoted "path" or <path> to be included and a boolean that is true
+/// if the include can be added, and false if:
----------------
why the change from empty string here? does this anticipate adding the path to the detail/doc unconditionally in a future patch?

This is quite a hard signature to understand and describe. At first glance it seems we can just split function into `bool shouldAddInclude()` and `calculateIncludePath()`, they won't overlap?


================
Comment at: clangd/Headers.h:94
   /// the same as DeclaringHeader but must be provided.
-  Expected<Optional<TextEdit>> insert(const HeaderFile &DeclaringHeader,
-                                      const HeaderFile &InsertedHeader) const;
+  Expected<std::pair<std::string, Optional<TextEdit>>>
+  insert(const HeaderFile &DeclaringHeader,
----------------
this signature is scary :-)

One option is to separate:
- `bool shouldInsert(Declaring, Inserted)` (probably just log errors and return false)
- `string spell(Inserted)`
- `TextEdit insert(Spelled)`

another is a struct, but I think making the flow a little more explicit at the callsite might be clearer.



Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D48163