[clangd] Simplify the code in XRefs

[ilya-biryukov]

Summary

While here, also fix potential UB in MergeIndex.

Thanks Kadir for finding this!

[merge_guards_bot]

Build result: pass - 60074 tests passed, 0 failed and 729 were skipped.
Log files: console-log.txt, CMakeCache.txt

[hokein]

Thanks, the title is a bit confusing, the modified code is about the index, not XRefs.

clang-tools-extra/clangd/index/Merge.cpp

116

I assume the potential UB is this code?

[ilya-biryukov]

clang-tools-extra/clangd/index/Merge.cpp

116

Yes, exactly. I believe it's not UB after C++17 or C++20.
However, mutating More inside the assignment to More definitely makes the reader pause, we'd rather avoid it.

clang-tools-extra/clangd/index/MemIndex.cpp

void MemIndex::lookup(const LookupRequest &Req,
   }
 }
 
 bool MemIndex::refs(const RefsRequest &Req,
                     llvm::function_ref<void(const Ref &)> Callback) const {
   trace::Span Tracer("MemIndex refs");
   uint32_t Remaining =
       Req.Limit.getValueOr(std::numeric_limits<uint32_t>::max());
-  bool More = false;
   for (const auto &ReqID : Req.IDs) {
     auto SymRefs = Refs.find(ReqID);
     if (SymRefs == Refs.end())
       continue;
     for (const auto &O : SymRefs->second) {
       if (!static_cast<int>(Req.Filter & O.Kind))
         continue;
-      if (Remaining == 0) {
-        More = true;
-        break;
-      }
-      if (Remaining > 0) {
-        --Remaining;
-        Callback(O);
-      }
-    }
-  }
-  return More;
+      if (Remaining == 0)
+        return true; // More refs were available.
+  return false; // We reported all refs.
 }
 
 void MemIndex::relations(
     const RelationsRequest &Req,
     llvm::function_ref<void(const SymbolID &, const Symbol &)> Callback) const {
   uint32_t Remaining =
       Req.Limit.getValueOr(std::numeric_limits<uint32_t>::max());
   for (const SymbolID &Subject : Req.Subjects) {

clang-tools-extra/clangd/index/Merge.cpp

bool MergedIndex::refs(const RefsRequest &Req,
   //
   // FIXME: The heuristic fails if the dynamic index contains a file, but all
   // refs were removed (we will report stale ones from the static index).
   // Ultimately we should explicit check which index has the file instead.
   llvm::StringSet<> DynamicIndexFileURIs;
   More |= Dynamic->refs(Req, [&](const Ref &O) {
     DynamicIndexFileURIs.insert(O.Location.FileURI);
     Callback(O);
+    assert(Remaining != 0);
     --Remaining;
   });
   if (Remaining == 0 && More)
     return More;
   // We return less than Req.Limit if static index returns more refs for dirty
   // files.
-  More |= Static->refs(Req, [&](const Ref &O) {
+  bool StaticHadMore =  Static->refs(Req, [&](const Ref &O) {
     if (DynamicIndexFileURIs.count(O.Location.FileURI))
       return; // ignore refs that have been seen from dynamic index.
-    if (Remaining == 0)
+    if (Remaining == 0) {
       More = true;
-    if (Remaining > 0) {
-      --Remaining;
-      Callback(O);
-    }
+      return;
+    }
   });
-  return More;
+  return More || StaticHadMore;
 }
 
 void MergedIndex::relations(
     const RelationsRequest &Req,
     llvm::function_ref<void(const SymbolID &, const Symbol &)> Callback) const {
   uint32_t Remaining =
       Req.Limit.getValueOr(std::numeric_limits<uint32_t>::max());
   // Return results from both indexes but avoid duplicates.

clang-tools-extra/clangd/index/dex/Dex.cpp

void Dex::lookup(const LookupRequest &Req,
   }
 }
 
 bool Dex::refs(const RefsRequest &Req,
                llvm::function_ref<void(const Ref &)> Callback) const {
   trace::Span Tracer("Dex refs");
   uint32_t Remaining =
       Req.Limit.getValueOr(std::numeric_limits<uint32_t>::max());
-  bool More = false;
   for (const auto &ID : Req.IDs)
     for (const auto &Ref : Refs.lookup(ID)) {
       if (!static_cast<int>(Req.Filter & Ref.Kind))
         continue;
-      if (Remaining == 0) {
-        More = true;
-        break;
-      }
-      if (Remaining > 0) {
-        --Remaining;
-        Callback(Ref);
-      }
-    }
-  return More;
+      if (Remaining == 0)
+        return true; // More refs were available.
+  return false; // We reported all refs.
 }
 
 void Dex::relations(
     const RelationsRequest &Req,
     llvm::function_ref<void(const SymbolID &, const Symbol &)> Callback) const {
   trace::Span Tracer("Dex relations");
   uint32_t Remaining =
       Req.Limit.getValueOr(std::numeric_limits<uint32_t>::max());