[PATCH] D50898: [clangd] Suggest code-completions for overriding base class virtual methods.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Looks good mostly, a few nits. We should make sure all related comments are updated accordingly



================
Comment at: clangd/CodeComplete.cpp:198
+static std::vector<CodeCompletionResult>
+getVirtualNonOverridenMethods(const DeclContext *DC, Sema *S) {
+  const auto *CR = llvm::dyn_cast<CXXRecordDecl>(DC);
----------------
Since we are returning `CodeCompletionResult`, maybe naming it like `getNonOverridenCompleteionResults` is clearer?


================
Comment at: clangd/CodeComplete.cpp:222
+      const std::string Name = Method->getNameAsString();
+      const auto it = Overrides.find(Name);
+      bool IsOverriden = false;
----------------
nit: Can't we use `Overrides.find(Method->getName())`  and the other places as well?


================
Comment at: clangd/CodeComplete.cpp:224
+      bool IsOverriden = false;
+      if (it != Overrides.end())
+        for (auto *MD : it->second) {
----------------
nit: use `{}` around the body of `if` -- the one-line for statement is across line, adding `{}` around it will improve the readability. 


================
Comment at: clangd/CodeComplete.cpp:1238
                             : SymbolSlab();
     // Merge Sema and Index results, score them, and pick the winners.
+    const auto Overrides = getVirtualNonOverridenMethods(
----------------
nit: we need to update the comment accordingly.


================
Comment at: clangd/CodeComplete.cpp:1281
   // Groups overloads if desired, to form CompletionCandidate::Bundles.
   // The bundles are scored and top results are returned, best to worst.
   std::vector<ScoredBundle>
----------------
here as well.


================
Comment at: clangd/CodeComplete.cpp:1322
       AddToBundles(&SemaResult, CorrespondingIndexResult(SemaResult));
+    for (auto &OverrideResult : OverrideResults)
+      AddToBundles(&OverrideResult, CorrespondingIndexResult(OverrideResult),
----------------
IIUC, we are treating the override results the same `SemaResult`, it is safe as long as Sema doesn't provide these overrides in its code completion results, otherwise we will have duplicated completion items?

I think we probably need a proper comment explaining what's going on here.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50898