Return insertion point from List::binarySearch() misses#10048
Return insertion point from List::binarySearch() misses#10048cmarcelo wants to merge 3 commits intoshader-slang:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can get early access to new features in CodeRabbit.Enable the |
There was a problem hiding this comment.
Pull request overview
Updates List::binarySearch() to return a useful insertion-point encoding on misses (bitwise negation of insertion index), aligns an existing caller with the new semantics, and adds coverage to prevent regressions.
Changes:
- Change
List::binarySearch()miss behavior from always-1to~insertionIndex. - Update language-server formatting logic to treat “found” as
>= 0instead of!= -1. - Add unit tests that validate both “found” indices and insertion-point results for default and custom comparers.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
source/core/slang-list.h |
Implements the new “miss returns ~insertionIndex” binarySearch() contract. |
source/slang/slang-language-server-auto-format.cpp |
Fixes a caller that previously assumed “not found” meant -1. |
tools/slang-unit-test/unit-test-list.cpp |
Adds regression tests covering found/miss behavior and insertion usage. |
source/slang/slang-check-decl.cpp |
Uses _compareDeclsInCommonParentByOrderOfDeclaration() for stable decl ordering in the updated area. |
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 (1)
source/slang/slang-check-decl.cpp (1)
12293-12379:⚠️ Potential issue | 🟠 MajorGuard against ancestor/descendant inputs before asserting parentage.
The early return path in
findDeclsLowestCommonAncestor()(line 12277) modifies one of its by-reference parameters without ensuring both parameters remain direct children of the returned ancestor. Specifically, when one declaration is a direct child of another, the function returns the grandparent but leaves one parameter pointing to the child—violating the precondition that bothlhsandrhsare direct children ofcommonParent. This trips the assertions at lines 12303–12304, or causesUNREACHABLEat line 12378.Add a guard after the LCA call to handle cases where the result fails this precondition:
Defensive guard
static int _compareDeclsInCommonParentByOrderOfDeclaration( ContainerDecl* commonParent, Decl* lhs, Decl* rhs) { if (lhs == rhs) return 0; SLANG_ASSERT(commonParent); SLANG_ASSERT(lhs); SLANG_ASSERT(rhs); - SLANG_ASSERT(lhs->parentDecl == commonParent); - SLANG_ASSERT(rhs->parentDecl == commonParent); + if (lhs->parentDecl != commonParent || rhs->parentDecl != commonParent) + { + // One decl is an ancestor of the other or not directly under commonParent. + // Fall back to deterministic comparison by depth and name. + auto depthToParent = [&](Decl* d) + { + Index depth = 0; + while (d && d->parentDecl && d->parentDecl != commonParent) + { + d = d->parentDecl; + ++depth; + } + return depth; + }; + int res = compareThreeWays(depthToParent(lhs), depthToParent(rhs)); + if (res) + return res; + return comparePtrs( + lhs->getName(), + rhs->getName(), + [](Name const& lName, Name const& rName) + { return strcmp(lName.text.begin(), rName.text.begin()); }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@source/slang/slang-check-decl.cpp` around lines 12293 - 12379, findDeclsLowestCommonAncestor can return a commonParent while one of the by-ref parameters (lhs/rhs) still refers to a descendant rather than a direct child, which breaks the precondition in _compareDeclsInCommonParentByOrderOfDeclaration that both have parentDecl == commonParent; after calling findDeclsLowestCommonAncestor, defend by walking any descendant argument up to its immediate child of commonParent (i.e. while(arg->parentDecl != commonParent) arg = arg->parentDecl;) or otherwise normalize lhs/rhs so that lhs->parentDecl and rhs->parentDecl match commonParent before asserting/entering _compareDeclsInCommonParentByOrderOfDeclaration; ensure you also guard against null and infinite loops by checking parentDecl is non-null.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@source/slang/slang-check-decl.cpp`:
- Around line 12293-12379: findDeclsLowestCommonAncestor can return a
commonParent while one of the by-ref parameters (lhs/rhs) still refers to a
descendant rather than a direct child, which breaks the precondition in
_compareDeclsInCommonParentByOrderOfDeclaration that both have parentDecl ==
commonParent; after calling findDeclsLowestCommonAncestor, defend by walking any
descendant argument up to its immediate child of commonParent (i.e.
while(arg->parentDecl != commonParent) arg = arg->parentDecl;) or otherwise
normalize lhs/rhs so that lhs->parentDecl and rhs->parentDecl match commonParent
before asserting/entering _compareDeclsInCommonParentByOrderOfDeclaration;
ensure you also guard against null and infinite loops by checking parentDecl is
non-null.
a0f96e6 to
fa99047
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@source/slang/slang-check-decl.cpp`:
- Around line 3581-3582: The code computes subIndex and supIndex via
ancestor->getMembers().indexOf(subAncestor/supAncestor) but doesn’t handle
indexOf returning -1; if either is <0 the subtraction yields bogus ordering and
triggers the canonical order diagnostic incorrectly. Update the logic around the
indexOf calls (variables subIndex and supIndex) to assert presence or,
preferably, check for <0 and in that case fall back to calling compareDecls(…)
(or otherwise skip the ordering subtraction) so you only use the index
subtraction when both indices are >= 0; ensure the canonical-order diagnostic
path uses the fallback when a member isn’t found.
In `@tools/slang-unit-test/unit-test-list.cpp`:
- Around line 8-89: Add explicit edge-case tests in the SLANG_UNIT_TEST(list)
block to cover empty-list, single-element, and duplicate-value behavior: call
List<T>::binarySearch on an empty List<int> and assert it returns ~0 using
SLANG_CHECK; create a single-element List<int> and verify binarySearch finds the
existing value (>=0) and returns ~0 for a missing value, then insert and
re-check; create a List<int> with duplicate values and verify binarySearch finds
an index within the duplicates and that insert/insertIndex semantics (using
values.insert and ~searchResult) keep the list sorted. Reference the existing
List, binarySearch, insert, and SLANG_CHECK usages to mirror style and
assertions.
| auto subIndex = ancestor->getMembers().indexOf(subAncestor); | ||
| auto supIndex = ancestor->getMembers().indexOf(supAncestor); |
There was a problem hiding this comment.
Guard against indexOf misses before subtracting.
indexOf returns -1 if either ancestor isn’t present in the member list; that collapses ordering (or yields a bogus diff) and can trigger the “canonical order” diagnostic incorrectly. Consider asserting presence or falling back to compareDecls when either index is < 0.
💡 Suggested defensive fallback
- auto subIndex = ancestor->getMembers().indexOf(subAncestor);
- auto supIndex = ancestor->getMembers().indexOf(supAncestor);
+ auto subIndex = ancestor->getMembers().indexOf(subAncestor);
+ auto supIndex = ancestor->getMembers().indexOf(supAncestor);
+ if (subIndex < 0 || supIndex < 0)
+ {
+ return compareDecls(subAncestor, supAncestor);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto subIndex = ancestor->getMembers().indexOf(subAncestor); | |
| auto supIndex = ancestor->getMembers().indexOf(supAncestor); | |
| auto subIndex = ancestor->getMembers().indexOf(subAncestor); | |
| auto supIndex = ancestor->getMembers().indexOf(supAncestor); | |
| if (subIndex < 0 || supIndex < 0) | |
| { | |
| return compareDecls(subAncestor, supAncestor); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@source/slang/slang-check-decl.cpp` around lines 3581 - 3582, The code
computes subIndex and supIndex via
ancestor->getMembers().indexOf(subAncestor/supAncestor) but doesn’t handle
indexOf returning -1; if either is <0 the subtraction yields bogus ordering and
triggers the canonical order diagnostic incorrectly. Update the logic around the
indexOf calls (variables subIndex and supIndex) to assert presence or,
preferably, check for <0 and in that case fall back to calling compareDecls(…)
(or otherwise skip the ordering subtraction) so you only use the index
subtraction when both indices are >= 0; ensure the canonical-order diagnostic
path uses the fallback when a member isn’t found.
fa99047 to
f169b79
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/slang-unit-test/unit-test-list.cpp`:
- Around line 119-122: The inline comment is wrong: it says "Insert another 20"
but the test actually inserts 15; update the comment to accurately describe the
operation (e.g., "Insert another 15 using the standard insert pattern") so
readers match the intent to the code around Index searchResult =
values.binarySearch(15); and values.insert(~searchResult, 15);.
09e78cc to
dd7bfae
Compare
dd7bfae to
273966b
Compare
The binarySearch() was comparing pointer addresses on the member list that is ordered by the declaration position. That may not necessarily work unless is guaranteed that the allocator produces pointers in declaration order and they are not reordered. Use indexOf() instead. See bfae49d ("Mediate access to ContainerDecl members (shader-slang#7242)") for context.
273966b to
4ca8b0d
Compare
Automated review dismissed: bot reviews must use COMMENT only, not APPROVE or REQUEST_CHANGES.
Change binarySearch() to return the bitwise negation of the insertion index when the element is not found, instead of always returning -1. Update call-site that was assuming -1 as the error value.
4ca8b0d to
eeb7f47
Compare
Automated review dismissed: bot reviews must use COMMENT only, not APPROVE or REQUEST_CHANGES.
There was a problem hiding this comment.
Review Summary
Clean, well-scoped PR that fulfills the existing TODO in binarySearch() by returning ~imin (bitwise negation of the insertion index) instead of -1 on misses. This follows the well-established .NET Array.BinarySearch convention.
Correctness
binarySearch() implementation (slang-list.h): The ~imin return is correct. When the search loop terminates without a match, imin always holds the index where the element should be inserted to maintain sorted order. The bitwise negation guarantees a negative value for any non-negative insertion index, and ~(~imin) == imin round-trips cleanly for callers.
slang-check-decl.cpp: The switch from binarySearch to indexOf is actually a pre-existing bug fix — the original code applied binary search to getMembers() (a List<Decl*> not sorted by pointer address), so the comparison was meaningless. indexOf with pointer equality is the correct approach, consistent with how _compareDeclsInCommonParentByOrderOfDeclaration does the same kind of member-order lookup via linear iteration.
slang-language-server-auto-format.cpp: The != -1 → >= 0 update correctly adapts to the new convention.
Call-site audit
I verified there are exactly two production call sites of binarySearch in the codebase, both addressed by this PR. No missed callers.
Build system
The new unit-test-list.cpp file is auto-discovered by slang_glob_sources in the CMake build — no CMakeLists.txt change needed, consistent with all other unit test files.
Tests
Thorough unit tests covering basic hits/misses, insert-and-verify round-trips, custom comparers, empty lists, single elements, and duplicates. The insertAndCheck pattern effectively demonstrates the intended API usage.
Minor suggestion
The inline comment could be slightly expanded to document the full return-value contract (see inline comment), but this is purely a nit — the code is clear and the tests serve as living documentation.
LGTM 👍
|
|
||
| auto subIndex = ancestor->getMembers().binarySearch(subAncestor); | ||
| auto supIndex = ancestor->getMembers().binarySearch(supAncestor); | ||
| auto subIndex = ancestor->getMembers().indexOf(subAncestor); |
There was a problem hiding this comment.
Good fix. The original code was using binarySearch with the default comparer (which compares Decl* pointer values), but getMembers() is not sorted by pointer address. indexOf (linear scan with pointer equality) is the correct approach here, and is consistent with how _compareDeclsInCommonParentByOrderOfDeclaration (line ~12790) does the same kind of member-order lookup via linear iteration.
| } | ||
| // TODO: The return value on a failed search should be | ||
| // the bitwise negation of the index where `obj` should | ||
| // be inserted to be in the proper sorted location. | ||
| return -1; | ||
| // If not found, return the bitwise negation of the insertion index. |
There was a problem hiding this comment.
nit: Consider expanding the comment slightly to document the full return-value contract at the method level, since callers need to know how to interpret both success and failure cases. Something like:
// If not found, return the bitwise negation of the insertion index.
// Callers can test result < 0 for "not found", and use ~result to obtain the insertion point.
This is minor — the existing comment is accurate, and the unit tests serve as living documentation for the convention.
| return 0; | ||
| }); | ||
| if (exclusionRangeId != -1) | ||
| if (exclusionRangeId >= 0) |
There was a problem hiding this comment.
Correct update. With the new binarySearch semantics, >= 0 is the canonical way to test for "found". 👍
| // unit-test-list.cpp | ||
|
|
||
| #include "core/slang-basic.h" | ||
| #include "unit-test/slang-unit-test.h" | ||
|
|
||
| using namespace Slang; | ||
|
|
||
| namespace | ||
| { | ||
| template<typename T, typename GetValueFunc> | ||
| void checkListIsSortedBy(const List<T>& list, const GetValueFunc& getValue) | ||
| { | ||
| for (Index i = 1; i < list.getCount(); i++) | ||
| SLANG_CHECK(getValue(list[i - 1]) <= getValue(list[i])); | ||
| } | ||
|
|
||
| template<typename T> | ||
| void checkListIsSorted(const List<T>& list) | ||
| { | ||
| checkListIsSortedBy(list, [](const T& value) { return value; }); | ||
| } | ||
| } // namespace | ||
|
|
||
| SLANG_UNIT_TEST(list) | ||
| { | ||
| { | ||
| List<int> values = {10, 20, 30, 40}; | ||
|
|
||
| SLANG_CHECK(values.binarySearch(5) == ~0); | ||
| SLANG_CHECK(values.binarySearch(10) == 0); | ||
| SLANG_CHECK(values.binarySearch(25) == ~2); | ||
| SLANG_CHECK(values.binarySearch(30) == 2); | ||
| SLANG_CHECK(values.binarySearch(50) == ~4); | ||
|
|
||
| auto insertAndCheck = [&](int queryValue, Index expectedInsertIndex) | ||
| { | ||
| Index searchResult = values.binarySearch(queryValue); | ||
| SLANG_CHECK(searchResult == ~expectedInsertIndex); | ||
|
|
||
| Index insertIndex = ~searchResult; | ||
| values.insert(insertIndex, queryValue); | ||
|
|
||
| checkListIsSorted(values); | ||
|
|
||
| Index foundIndex = values.binarySearch(queryValue); | ||
| SLANG_CHECK(foundIndex >= 0); | ||
| SLANG_CHECK(values[foundIndex] == queryValue); | ||
| }; | ||
|
|
||
| insertAndCheck(5, 0); | ||
| insertAndCheck(25, 3); | ||
| insertAndCheck(50, 6); | ||
| } | ||
|
|
||
| { | ||
| struct Entry | ||
| { | ||
| int key; | ||
| }; | ||
|
|
||
| List<Entry> entries; | ||
| entries.add(Entry{1}); | ||
| entries.add(Entry{4}); | ||
| entries.add(Entry{9}); | ||
|
|
||
| auto comparer = [](const Entry& entry, int key) | ||
| { | ||
| if (entry.key < key) | ||
| return -1; | ||
| if (entry.key > key) | ||
| return 1; | ||
| return 0; | ||
| }; | ||
|
|
||
| auto insertAndCheck = [&](int queryKey, Index expectedInsertIndex) | ||
| { | ||
| Index searchResult = entries.binarySearch(queryKey, comparer); | ||
| SLANG_CHECK(searchResult == ~expectedInsertIndex); | ||
|
|
||
| Index insertIndex = ~searchResult; | ||
| entries.insert(insertIndex, Entry{queryKey}); | ||
|
|
||
| checkListIsSortedBy(entries, [](const Entry& entry) { return entry.key; }); | ||
|
|
||
| Index foundIndex = entries.binarySearch(queryKey, comparer); | ||
| SLANG_CHECK(foundIndex >= 0); | ||
| SLANG_CHECK(entries[foundIndex].key == queryKey); | ||
| }; | ||
|
|
||
| SLANG_CHECK(entries.binarySearch(0, comparer) == ~0); | ||
| SLANG_CHECK(entries.binarySearch(4, comparer) == 1); | ||
| SLANG_CHECK(entries.binarySearch(6, comparer) == ~2); | ||
| SLANG_CHECK(entries.binarySearch(10, comparer) == ~3); | ||
|
|
||
| insertAndCheck(0, 0); | ||
| insertAndCheck(6, 3); | ||
| insertAndCheck(10, 5); | ||
| } | ||
|
|
||
| // Empty list | ||
| { | ||
| List<int> values; | ||
| SLANG_CHECK(values.binarySearch(42) == ~0); | ||
| } | ||
|
|
||
| // Single element | ||
| { | ||
| List<int> values = {10}; | ||
| SLANG_CHECK(values.binarySearch(10) >= 0); | ||
| SLANG_CHECK(values.binarySearch(5) == ~0); | ||
| SLANG_CHECK(values.binarySearch(15) == ~1); | ||
|
|
||
| Index searchResult = values.binarySearch(5); | ||
| values.insert(~searchResult, 5); | ||
| SLANG_CHECK(values.getCount() == 2); | ||
| checkListIsSorted(values); | ||
| SLANG_CHECK(values[0] == 5); | ||
| SLANG_CHECK(values[1] == 10); | ||
| SLANG_CHECK(values.binarySearch(5) >= 0); | ||
| } | ||
|
|
||
| // Duplicate values | ||
| { | ||
| List<int> values = {10, 20, 20, 30}; | ||
|
|
||
| Index foundIndex = values.binarySearch(20); | ||
| SLANG_CHECK(foundIndex >= 0); | ||
| SLANG_CHECK(values[foundIndex] == 20); | ||
|
|
||
| Index searchResult = values.binarySearch(15); | ||
| SLANG_CHECK(searchResult < 0); | ||
| values.insert(~searchResult, 15); | ||
|
|
||
| checkListIsSorted(values); | ||
|
|
||
| SLANG_CHECK(values.binarySearch(15) >= 0); | ||
| } | ||
| } |
There was a problem hiding this comment.
Excellent test coverage. The tests exercise:
- Basic hits and misses with exact insertion-point verification
- Insert-and-verify round-trip pattern (the primary use case for the new semantics)
- Custom comparers with heterogeneous types
- Empty list, single element, and duplicate value edge cases
The insertAndCheck lambda pattern nicely demonstrates the intended usage of ~result as an insertion index.
Automated review dismissed: bot reviews must use COMMENT only, not APPROVE or REQUEST_CHANGES.
|
@csyonghe Do you have review bandwidth? Please suggest an alternate reviewer otherwise. |
jhelferty-nv
left a comment
There was a problem hiding this comment.
Thanks for your contribution! I'm thinking it would be good to have a doc comment for binarySearch and possibly a change to the return type? Details inline.
| inline void swapElements(Index index1, Index index2) { swapElements(m_buffer, index1, index2); } | ||
|
|
||
| template<typename T2, typename Comparer> | ||
| Index binarySearch(const T2& obj, Comparer comparer) const |
There was a problem hiding this comment.
So binarySearch doesn't have a doc comment, which feels bad when you're changing the meaning of Index like this. But that still leaves it to the user to make sure they check the docs/know what they're doing.
I'm wondering if using something like the following as the return type might be better:
struct FindIndex
{
Index rawValue;
bool wasFound() const { return rawValue >= 0; }
Index getIndex() const { SLANG_ASSERT(wasFound()); return rawValue; }
Index getInsertIndex() const { SLANG_ASSERT(!wasFound()); return ~rawValue; }
};That should compile down to the same thing in release builds? I think it might be better, as long as it doesn't make call sites more complex/confusing. Thoughts?
There was a problem hiding this comment.
Instead of growing the return type, maybe split up the functionality into two functions, this follows what C++ standard library does:
binarySearch(x): same interface as before, either index or -1 if not found.lowerBound(x): returns the index of first element that is >= x, which effectively returns the insertion point. If duplicates are unwanted, the caller can check if the item is present or not.
I prefer that this puts the burden in the new use case, and lower-bound is a common functionality in various other APIs.
What do you think?
There was a problem hiding this comment.
It would be something like this -> cmarcelo@b02c88c
There was a problem hiding this comment.
I am struggling to understand the utility of this change, then. We only have a few call sites that use binarySearch, and as far as I can tell, we're not making use of the insertion index at any of them. So while this overall change does address a TODO, it's adding functionality that's not actually being used, and isn't needed at any of the call sites. From that perspective, adding a lowerBound function that is unused would also not be useful.
For what it's worth, I did talk to Tess about this (who wrote the TODO) and she said that it was likely inspired by another language doing this. (C#?)
I suggest we close this PR.
There was a problem hiding this comment.
I'll use the functionality in #10076 (Expanding Metadata API).
My understanding is that Slang GitHub policy is to squash commits in the PRs. Since this is a relatively independent and general purpose change, which also bootstrapped a test file for List, it felt like a candidate to split into its own PR.
I'm fine closing this PR if preferred, but the same code change will be needed in one way or another in the Metadata PR.
There was a problem hiding this comment.
Added lowerBound as a local function in the call-site. I'll close this for now, if more use cases appear I'll re-open this or open a PR to move lowerBound there.
|
Added lowerBound as a local function in the call-site. I'll close this for now, if more use cases appear I'll re-open this or open a PR to move lowerBound there. |
Change binarySearch() to return the bitwise negation of the insertion
index when the element is not found, instead of always returning -1.
Update call-site that was assuming -1 as the error value.
On top of #10047
Compiler Impact
This PR updates an internal container utility and its call sites; it affects internal semantic checks and language-server formatting logic but does not change lexer, parser, IR, or codegen.
Compiler stages affected:
source/slang/slang-check-decl.cppupdated to use the newList::binarySearch()semantics (some call-sites switched toindexOfor otherwise adjusted) when computing declaration/member indices for checks (e.g., generic constraint comparisons).source/slang/slang-language-server-auto-format.cppupdated to treat non-foundbinarySearchresults correctly (changed check from!= -1to>= 0).Target backends impacted:
Public API headers:
Changes Summary
source/core/slang-list.hList::binarySearch()now returns the bitwise negation of the insertion index (~imin) when an element is not found, instead of-1. Comments updated to document the behavior.source/slang/slang-check-decl.cppindexOfor updated to handle the new return convention) so computed indices/differences remain correct.source/slang/slang-language-server-auto-format.cppexclusionRangeId != -1toexclusionRangeId >= 0to correctly detect matches under the new convention.tools/slang-unit-test/unit-test-list.cppList::binarySearch()for found results and insertion-point semantics (including custom comparers, empty/single/duplicate-element cases) and verifying insertion behavior using the~resultpattern.Notes