[PATCH] D41537: Optionally add code completion results for arrow instead of dot

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Sorry for not taking a look for a long time. I think that would be a very useful feature to have.
Here are a few comments (also see the inline comments)

- Please add tests
- How would the clients know how to correct dot to arrow? It'd be cool if code completion results to provide a replacement for that (e.g. return a `FixItHint` that should be applied along with the completion items that have `RequiresDotToArrowCorrection == true`).
- We should probably have a more general option, i.e. `AllowCorrections`. We could reuse it for other advanced completion scenarios.
- This patch calls `HandleCodeCompleteResults` twice and it breaks at least one existing client (clangd) and, arguably, makes the contract of `CodeCompleteConsumer` more complicated. I have a patch that addresses that: https://reviews.llvm.org/D42474. The change also moves some of the code to parser, arguably making the overall patch.



================
Comment at: include/clang/Sema/CodeCompleteConsumer.h:493
+                       StringRef ParentName, const char *BriefComment,
+                       bool RequiresDotToArrowCorrection = false);
   ~CodeCompletionString() = default;
----------------
Maybe remove the default argument?
This constructor seems to only be called `CodeCompletionBuilder::TakeString`.


================
Comment at: include/clang/Sema/CodeCompleteConsumer.h:595
   const char *BriefComment;
+  bool RequiresDotToArrowCorrection = false;
   
----------------
Maybe remove `= false`, initialize in constructor to be consistent with how other members are initialized?


================
Comment at: include/clang/Sema/CodeCompleteConsumer.h:612
+                        unsigned Priority, CXAvailabilityKind Availability,
+                        bool RequiresDotToArrowCorrection = false)
     : Allocator(Allocator), CCTUInfo(CCTUInfo),
----------------
Maybe remove the default argument too?
This method seems to have only 5 callsites, they should be very easy to update.


================
Comment at: include/clang/Sema/CodeCompleteOptions.h:43
+  /// Show also results after dot to arrow correction if arrow operator can be applied.
+  unsigned TryArrowInsteadOfDot : 1;
+
----------------
Could we make a more general option (say, `IncludeCorrections`)?
Arrow instead of dot is useful, but we might try to add more items that require various corrections when doing completions and it would be nice to reuse the option between those.


================
Comment at: lib/Sema/SemaCodeComplete.cpp:4066
+  } else {
+    const DeclarationName ArrowOpName =
+        SemaRef.Context.DeclarationNames.getCXXOperatorName(OO_Arrow);
----------------
The code here seems to replicate what the parser does, so certifying that's it's correct is hard and we may not pick up the changes when something changes in parser.
I suggest we compute the `Expr* Base` for an alternate operator and pass it to code complete function. Here's a change that does that, feel free to pick it up unless you have objections to my line of thought: D42474 (it's missing marking the results with proper flags, so that's something that should be extended).


https://reviews.llvm.org/D41537