perf(registry): O(1) symbol row lookup#1847
Conversation
WalkthroughThe PR refactors symbol storage in ChangesSymbol Storage Container Refactoring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Microsoft Presidio Analyzer (2.2.362)src/libs/registry/docset.cppMicrosoft Presidio Analyzer failed to scan this file Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/libs/registry/docset.cpp (2)
304-310: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
QMap::operator[]in aconstmethod silently inserts an empty list for unknown symbol types.When
symbolTypeis not inm_symbolStrings,loadSymbols(symbolType)is a no-op, som_symbolsstill doesn't contain the key after the call. Line 309 then uses the non-constoperator[](permitted becausem_symbolsismutable) which inserts a default-constructedQList, acting as negative caching. This is semantically correct but can be made explicit by first callingloadSymbols, which already creates the entry in the two-arg overload, or by usingfind/constFindto avoid the implicit insertion. At minimum, a comment explaining the intentional negative-cache insertion would prevent future confusion.♻️ Proposed clarification — use `find` to make intent explicit
const QList<std::pair<QString, QUrl>> &Docset::symbols(const QString &symbolType) const { if (!m_symbols.contains(symbolType)) { loadSymbols(symbolType); + // Ensure an entry exists even if symbolType has no DB aliases (negative cache). + m_symbols[symbolType]; // default-constructs empty list if still absent } - return m_symbols[symbolType]; + return *m_symbols.find(symbolType); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libs/registry/docset.cpp` around lines 304 - 310, The Docset::symbols const method currently uses m_symbols[symbolType] which, due to m_symbols being mutable, silently inserts a default QList for unknown keys; update the method to avoid implicit insertion by first calling loadSymbols(symbolType) and then retrieving the entry with m_symbols.constFind(symbolType) (or m_symbols.find/check contains) and return the found const reference if present, otherwise return a static empty QList reference; mention or adjust loadSymbols and m_symbolStrings usage so the two-arg overload still creates entries when intended to preserve explicit negative-caching behavior.
528-567:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSorting regression: multi-alias symbol types will no longer be globally sorted.
The old
QMultiMapguaranteed global sort order by key across all insertions. The new code callsloadSymbols(symbolType, symbolString)once per raw DB alias (e.g., all of"dcop","func","ffunc","signal","slot", … for the canonical"Function"type) and appends each query'sORDER BY nameresult to the sameQList. WithQMultiMap, the items are always sorted by key, so inserting from multiple queries still yielded a globally sorted list. With sequentialemplace_backappends the final list is only sorted within each alias's batch — not across batches — breaking alphabetical order in the symbol panel for any canonical type that has more than one raw DB alias.🐛 Proposed fix — sort after all batches are appended (one-arg overload)
void Docset::loadSymbols(const QString &symbolType) const { for (auto it = std::as_const(m_symbolStrings).equal_range(symbolType); it.first != it.second; ++it.first) { loadSymbols(symbolType, it.first.value()); } + + // Re-sort across alias batches; each batch is already sorted by the SQL ORDER BY, + // but multiple batches are appended sequentially, not merged. + auto &list = m_symbols[symbolType]; + std::sort(list.begin(), list.end(), + [](const auto &a, const auto &b) { return a.first < b.first; }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libs/registry/docset.cpp` around lines 528 - 567, The one-argument overload Docset::loadSymbols(const QString &symbolType) appends multiple alias batches into m_symbols[symbolType] without preserving a global sort; after the loop that calls loadSymbols(symbolType, it.first.value()), obtain a reference to QList<std::pair<QString,QUrl>>& symbols = m_symbols[symbolType] and sort it once (e.g. use std::sort with a lambda that compares the pair.first QStrings via QString::localeAwareCompare or QString::compare) to restore global alphabetical order across all alias batches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/libs/registry/docset.cpp`:
- Around line 304-310: The Docset::symbols const method currently uses
m_symbols[symbolType] which, due to m_symbols being mutable, silently inserts a
default QList for unknown keys; update the method to avoid implicit insertion by
first calling loadSymbols(symbolType) and then retrieving the entry with
m_symbols.constFind(symbolType) (or m_symbols.find/check contains) and return
the found const reference if present, otherwise return a static empty QList
reference; mention or adjust loadSymbols and m_symbolStrings usage so the
two-arg overload still creates entries when intended to preserve explicit
negative-caching behavior.
- Around line 528-567: The one-argument overload Docset::loadSymbols(const
QString &symbolType) appends multiple alias batches into m_symbols[symbolType]
without preserving a global sort; after the loop that calls
loadSymbols(symbolType, it.first.value()), obtain a reference to
QList<std::pair<QString,QUrl>>& symbols = m_symbols[symbolType] and sort it once
(e.g. use std::sort with a lambda that compares the pair.first QStrings via
QString::localeAwareCompare or QString::compare) to restore global alphabetical
order across all alias batches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dff2f217-2c67-4634-86dc-0700b7f221c6
📒 Files selected for processing (3)
src/libs/registry/docset.cppsrc/libs/registry/docset.hsrc/libs/registry/listmodel.cpp
1 participant
Summary by CodeRabbit