Conversation
Se añade el alias clear() como sinónimo de empty() en múltiples contenedores para mejorar la compatibilidad con STL y consistencia de API. También se agregan métodos contains() y contains_if() a contenedores secuenciales y se implementa is_empty() en grafos y tablas hash. Se incluyen pruebas unitarias para verificar la funcionalidad de clear() y operadores de igualdad en listas dinámicas.
Contributor
|
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:
WalkthroughAdds convenience APIs ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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)
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds clear() convenience aliases (and a few related query helpers) across Aleph-w containers to align APIs and improve ergonomics, plus corresponding unit tests.
Changes:
- Add
clear()as an alias forempty()across many containers (arrays, lists, heaps, hash tables, etc.). - Add
LocateFunctions::contains_if()andLocateFunctions::contains()helpers, and enable sequence equality forDynList/DynDlistviaEqualSequenceMethod. - Add/extend tests covering
clear(),contains*(), and sequence equality.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tpl_memArray.H | Adds clear() alias for empty() in MemArray. |
| tpl_linHash.H | Adds clear() alias for empty() in linear hash table. |
| tpl_lhash.H | Adds clear() alias for empty() in list-based hash table. |
| tpl_hash.H | Adds clear() and is_empty() wrappers for HashSet/HashMap. |
| tpl_graph.H | Adds Path::clear() alias for Path::empty(). |
| tpl_dyn_slist_nc.H | Adds clear() alias for empty() in Dyn_Slist_Nc. |
| tpl_dynSkipList.H | Normalizes clear() comment text. |
| tpl_dynSetTree.H | Adds clear() alias for empty() in DynSetTree. |
| tpl_dynSetHash.H | Adds clear() (and is_empty() wrapper) for dynamic hash table wrapper. |
| tpl_dynDlist.H | Adds EqualSequenceMethod base + clear() alias for empty() in DynDlist. |
| tpl_dynBinHeap.H | Adds clear() alias for empty() in DynBinHeap. |
| tpl_dynArray.H | Adds clear() alias for empty() in DynArray. |
| tpl_arrayStack.H | Adds clear() alias for empty() in array-backed stack(s). |
| tpl_arrayQueue.H | Adds clear() alias for empty() in array-backed queue. |
| tpl_array.H | Adds clear() alias for empty() in Array. |
| ringfilecache.H | Adds clear() alias for empty() in ring file cache. |
| quadtree.H | Adds clear() alias for empty() in quadtree. |
| htlist.H | Adds EqualSequenceMethod base + clear() alias for empty() in DynList. |
| hashDry.H | Adds clear() alias for empty() in open-addressing hash base. |
| graph-dry.H | Adds is_empty() accessor for graphs. |
| archeap.H | Adds clear() alias for empty() in arc heap types. |
| ah-dry.H | Adds contains_if() / contains() to LocateFunctions. |
| Tests/dynsethash.cc | Adds tests for clear() on hash table/map wrappers. |
| Tests/dynlist.cc | Adds tests for DynList::clear(), contains(), and equality operators. |
| Tests/dyndlist.cc | Adds tests for DynDlist equality operators. |
| Tests/dynarray.cc | Adds tests for DynArray::clear() and contains*(). |
| Tests/array.cc | Adds tests for Array::contains() and contains_if(). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
quadtree.H (1)
534-536: Implementation looks good; consider improving Doxygen consistency.The alias implementation is correct. For consistency with the rest of the file's documentation style, consider using the
@brieftag and adding a cross-reference toempty().📝 Optional documentation enhancement
- /// Alias for empty() - void clear() { empty(); } + /** `@brief` Clear all points from the tree. + * `@see` empty() + */ + void clear() { empty(); }As per coding guidelines: "If a new public method or class is added, a corresponding unit test MUST be created." Since
clear()is a trivial alias toempty(), you might add a simple test case that verifiesclear()behaves identically toempty(), or document why it's covered by existingempty()tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@quadtree.H` around lines 534 - 536, Replace the brief doc for clear() to match file style: add a Doxygen `@brief` tag and a cross-reference to empty() (e.g., "@brief Alias for empty(). See empty() for behavior.") so it is consistent with other comments; also add a tiny unit test exercising Quadtree::clear() that verifies it yields the same state as empty() (or add a test note explaining why existing empty() tests cover clear()) to satisfy the testing guideline.tpl_memArray.H (1)
349-351: Consider addingnoexceptspecifier for API consistency.The implementation is correct. However, since
empty()only performsn = 0(a non-throwing operation), bothempty()andclear()could be markednoexceptto provide stronger exception guarantees. This would also align withArray<T>::clear()andDynArray<T>::clear()which are markednoexcept.♻️ Optional: Add noexcept specifier
/// Empty the container. The array is not contracted - void empty() { n = 0; } + void empty() noexcept { n = 0; } /// Alias for empty() - void clear() { empty(); } + void clear() noexcept { empty(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tpl_memArray.H` around lines 349 - 351, Mark the non-throwing methods as noexcept for API consistency: add the noexcept specifier to tpl_memArray::empty() (since it only does n = 0) and to the alias method tpl_memArray::clear() (currently defined as void clear() { empty(); }) so their declarations/definitions match Array<T>::clear() and DynArray<T>::clear() semantics and provide stronger exception guarantees.graph-dry.H (1)
697-699: Nice to have: makeis_empty()constexprfor accessor consistency.Line 698 is a pure state check, like neighboring size accessors, so it can be
constexprtoo.♻️ Proposed tweak
- [[nodiscard]] bool is_empty() const noexcept { return num_nodes == 0; } + [[nodiscard]] constexpr bool is_empty() const noexcept { return num_nodes == 0; }As per coding guidelines: Use
constexprfor compile-time constants and functions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@graph-dry.H` around lines 697 - 699, Make the accessor is_empty() constexpr by changing its declaration from [[nodiscard]] bool is_empty() const noexcept to [[nodiscard]] constexpr bool is_empty() const noexcept so the empty check can be evaluated at compile time; update the member function signature for is_empty() (which reads num_nodes) and ensure num_nodes remains a constexpr-friendly member (no runtime-only requirements) so the function compiles as constexpr.Tests/array.cc (1)
357-366: Good test coverage, consider adding empty array edge case.The test appropriately validates both
contains()andcontains_if()methods with present/absent elements and matching/non-matching predicates.Per coding guidelines, tests should cover edge cases including empty inputs. Consider adding a test for an empty array to ensure both methods return
false.🧪 Suggested addition for empty array coverage
TEST(ArraySearch, Contains) { Array<int> arr = {10, 20, 30, 40}; EXPECT_TRUE(arr.contains(20)); EXPECT_TRUE(arr.contains(40)); EXPECT_FALSE(arr.contains(50)); EXPECT_TRUE(arr.contains_if([](int x) { return x > 25; })); EXPECT_FALSE(arr.contains_if([](int x) { return x > 100; })); + + Array<int> empty; + EXPECT_FALSE(empty.contains(1)); + EXPECT_FALSE(empty.contains_if([](int) { return true; })); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/array.cc` around lines 357 - 366, Add an edge-case test that verifies Array<T>::contains and Array<T>::contains_if return false for an empty Array; for example extend the TEST(ArraySearch, Contains) or add a new TEST(ArraySearch, ContainsEmpty) that constructs Array<int> arr = {} (or Array<int> empty) and asserts EXPECT_FALSE(empty.contains(whatever)) and EXPECT_FALSE(empty.contains_if([](int){ return true; })), referencing the Array class and its contains and contains_if methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ringfilecache.H`:
- Around line 870-872: The new public method clear() is missing a full Doxygen
contract; update the declaration of clear() to include a Doxygen block
documenting it as an alias of empty(): add `@brief` explaining that clear()
removes all entries, document that it has no parameters, add `@return` describing
the (void) result, include `@throws` (none) or note noexcept to indicate it never
throws, and mention it calls/aliases empty() so callers know equivalence to the
empty() method; reference the clear() method and the empty() implementation in
the docs.
- Around line 870-872: Add a unit test that exercises RingFileCache::clear() and
verifies it behaves identically to RingFileCache::empty(): set up a populated
RingFileCache, call clear(), then assert the cache state is emptied (e.g.,
size() == 0 and any isEmpty()/contains() checks match the empty() behavior);
also test that calling clear() on an already-empty cache is a no-op and matches
empty() results. Reference the RingFileCache::clear() and RingFileCache::empty()
methods in the test and include assertions for size() and any public inspection
APIs used to validate contents.
In `@tpl_dynBinHeap.H`:
- Around line 263-265: Add a full Doxygen contract for the new public method
clear() (which currently calls empty()), replacing the brief alias comment;
include `@brief` describing that clear() removes all elements from the heap,
`@param` noting there are none, `@return` void, and `@throws` documenting that it is
noexcept and thus throws nothing (or listing no exceptions), and reference that
it delegates to empty() so readers know behavior and exception-safety; ensure
tags and wording follow existing class (dynBinHeap) documentation style.
---
Nitpick comments:
In `@graph-dry.H`:
- Around line 697-699: Make the accessor is_empty() constexpr by changing its
declaration from [[nodiscard]] bool is_empty() const noexcept to [[nodiscard]]
constexpr bool is_empty() const noexcept so the empty check can be evaluated at
compile time; update the member function signature for is_empty() (which reads
num_nodes) and ensure num_nodes remains a constexpr-friendly member (no
runtime-only requirements) so the function compiles as constexpr.
In `@quadtree.H`:
- Around line 534-536: Replace the brief doc for clear() to match file style:
add a Doxygen `@brief` tag and a cross-reference to empty() (e.g., "@brief Alias
for empty(). See empty() for behavior.") so it is consistent with other
comments; also add a tiny unit test exercising Quadtree::clear() that verifies
it yields the same state as empty() (or add a test note explaining why existing
empty() tests cover clear()) to satisfy the testing guideline.
In `@Tests/array.cc`:
- Around line 357-366: Add an edge-case test that verifies Array<T>::contains
and Array<T>::contains_if return false for an empty Array; for example extend
the TEST(ArraySearch, Contains) or add a new TEST(ArraySearch, ContainsEmpty)
that constructs Array<int> arr = {} (or Array<int> empty) and asserts
EXPECT_FALSE(empty.contains(whatever)) and
EXPECT_FALSE(empty.contains_if([](int){ return true; })), referencing the Array
class and its contains and contains_if methods.
In `@tpl_memArray.H`:
- Around line 349-351: Mark the non-throwing methods as noexcept for API
consistency: add the noexcept specifier to tpl_memArray::empty() (since it only
does n = 0) and to the alias method tpl_memArray::clear() (currently defined as
void clear() { empty(); }) so their declarations/definitions match
Array<T>::clear() and DynArray<T>::clear() semantics and provide stronger
exception guarantees.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7904b48d-febe-4a74-a173-5d68e8423ce3
📒 Files selected for processing (27)
Tests/array.ccTests/dynarray.ccTests/dyndlist.ccTests/dynlist.ccTests/dynsethash.ccah-dry.Harcheap.Hgraph-dry.HhashDry.Hhtlist.Hquadtree.Hringfilecache.Htpl_array.Htpl_arrayQueue.Htpl_arrayStack.Htpl_dynArray.Htpl_dynBinHeap.Htpl_dynDlist.Htpl_dynSetHash.Htpl_dynSetTree.Htpl_dynSkipList.Htpl_dyn_slist_nc.Htpl_graph.Htpl_hash.Htpl_lhash.Htpl_linHash.Htpl_memArray.H
lrleon
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
Tests/quadtree_test.cc (1)
178-188: Consider adding edge case coverage forclear()on an empty tree.The test validates the basic
clear()behavior on a populated tree. To align with the test coverage guidelines, consider also testing:
clear()on an already-empty tree (no-op behavior)- Re-insertion after
clear()to confirm tree remains functionalThis mirrors the pattern used in
Tests/ringfilecache.ccwhere both populated and empty states are validated.💡 Suggested additions
TEST(QuadTreeBasic, ClearYieldsSameStateAsEmpty) { QuadTree tree(0, 100, 0, 100, 4); tree.insert(Point(25, 25)); tree.insert(Point(75, 75)); tree.clear(); EXPECT_EQ(tree.search(Point(25, 25)), nullptr); EXPECT_EQ(tree.search(Point(75, 75)), nullptr); + + // Verify tree is still usable after clear + Point * p = tree.insert(Point(50, 50)); + EXPECT_NE(p, nullptr); + EXPECT_NE(tree.search(Point(50, 50)), nullptr); + + // Clear on already-empty tree should be a no-op + tree.clear(); + tree.clear(); + EXPECT_EQ(tree.search(Point(50, 50)), nullptr); }As per coding guidelines: "Tests must cover edge cases: empty inputs, single element, large inputs."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/quadtree_test.cc` around lines 178 - 188, Add edge-case assertions around QuadTree::clear by adding a test that calls clear() on a newly constructed QuadTree (QuadTree tree(0, 100, 0, 100, 4)) and verifies search(Point(...)) returns nullptr (no-op behavior), and then test re-insertion after clear by inserting a Point, calling clear(), inserting again with QuadTree::insert and verifying search returns non-null for the re-inserted point; reference QuadTree, clear(), insert(), and search() to locate where to extend Tests/quadtree_test.cc.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htlist.H`:
- Around line 1711-1718: The Doxygen block for the member function clear()
noexcept incorrectly contains an `@param` tag for a non-existent parameter; remove
the line containing "@param none" (or replace it with a different applicable tag
such as a brief/note) so there are no `@param` entries for the parameterless
function clear(), leaving the `@brief` and `@return` lines (or removing `@return` void
if you prefer minimal docs) intact; locate the comment associated with the
clear() method (which delegates to empty()) and update it to eliminate the
`@param` none entry to avoid Doxygen warnings.
In `@tpl_dyn_slist_nc.H`:
- Around line 84-91: The Doxygen for Dyn_Slist_Nc::clear() incorrectly uses
"@param none" for a parameterless method; remove the "@param none" line from the
comment block for clear() (while keeping `@brief`, `@return` void and `@throws` none)
so the comment conforms to the guideline and produces no Doxygen warnings;
locate the clear() docblock in tpl_dyn_slist_nc.H and update it accordingly.
In `@tpl_graph.H`:
- Around line 2826-2833: The Doxygen block for Path::clear() incorrectly uses
"@param none" for a no-argument method which causes warnings; remove the "@param
none" tag from the comment above void clear() (keep the existing `@brief`, `@return`
void, and `@throws` none tags or adapt them to match project style) so the
docblock contains no `@param` entry for clear(), ensuring Doxygen emits no
warnings for this file.
---
Nitpick comments:
In `@Tests/quadtree_test.cc`:
- Around line 178-188: Add edge-case assertions around QuadTree::clear by adding
a test that calls clear() on a newly constructed QuadTree (QuadTree tree(0, 100,
0, 100, 4)) and verifies search(Point(...)) returns nullptr (no-op behavior),
and then test re-insertion after clear by inserting a Point, calling clear(),
inserting again with QuadTree::insert and verifying search returns non-null for
the re-inserted point; reference QuadTree, clear(), insert(), and search() to
locate where to extend Tests/quadtree_test.cc.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 17955a3b-f429-42b3-9cbb-cd3b9c03b77c
📒 Files selected for processing (29)
Tests/array.ccTests/quadtree_test.ccTests/ringfilecache.ccah-dry.Harcheap.Hgraph-dry.HhashDry.Hhtlist.Hquadtree.Hringfilecache.Htpl_array.Htpl_arrayQueue.Htpl_arrayStack.Htpl_dynArray.Htpl_dynBinHeap.Htpl_dynDlist.Htpl_dynListQueue.Htpl_dynListStack.Htpl_dynSetHash.Htpl_dynSetTree.Htpl_dynSkipList.Htpl_dyn_slist_nc.Htpl_graph.Htpl_hash.Htpl_lhash.Htpl_linHash.Htpl_memArray.Htpl_odhash.Htpl_olhash.H
✅ Files skipped from review due to trivial changes (2)
- tpl_dynListQueue.H
- tpl_dynListStack.H
🚧 Files skipped from review as they are similar to previous changes (13)
- quadtree.H
- tpl_dynBinHeap.H
- tpl_array.H
- hashDry.H
- tpl_dynDlist.H
- Tests/array.cc
- tpl_memArray.H
- tpl_linHash.H
- archeap.H
- tpl_lhash.H
- tpl_arrayStack.H
- tpl_hash.H
- tpl_dynSetHash.H
👮 Files not reviewed due to content moderation or server errors (7)
- tpl_dynArray.H
- tpl_odhash.H
- tpl_arrayQueue.H
- tpl_olhash.H
- ah-dry.H
- tpl_dynSkipList.H
- tpl_dynSetTree.H
…ameter descriptions
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tpl_dyn_slist_nc.H (1)
89-96: Remove invalid@param noneand consider addingnoexcept.Two issues with the new
clear()method:
@param noneis invalid Doxygen syntax for parameterless methods - remove this line.- The sibling
DynSlist::clear()is markednoexcept. For consistency and to signal thatclear()doesn't throw, consider addingnoexcept.Proposed fix
- /** `@brief` Alias for empty(). - - `@param` none - `@return` void - `@throws` none - `@see` empty() - */ - void clear() { empty(); } + /** `@brief` Alias for empty(). + * + * `@return` void + * `@throws` none + * `@see` empty() + */ + void clear() noexcept { empty(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tpl_dyn_slist_nc.H` around lines 89 - 96, Remove the invalid Doxygen "@param none" line from the clear() comment and mark the clear() method as noexcept to match the sibling DynSlist::clear(); specifically update the documentation block for clear() (which currently references empty()) by deleting the "@param none" entry and change the method signature void clear() { empty(); } to a noexcept version (e.g., void clear() noexcept { empty(); }) so the declaration and docs reflect that it cannot throw.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tpl_dyn_slist_nc.H`:
- Line 196: The method top() is declared const but calls the non-const
get_first(), causing const-correctness compilation errors; either remove const
from top() or (preferable) add a const overload of get_first() that can be
called on const Dyn_Slist_Nc objects (e.g., get_first() const returning const
T&) and keep top() as T top() const forwarding to that const get_first(); update
the get_first() declarations/definitions and any callers accordingly so const
Dyn_Slist_Nc instances compile.
---
Nitpick comments:
In `@tpl_dyn_slist_nc.H`:
- Around line 89-96: Remove the invalid Doxygen "@param none" line from the
clear() comment and mark the clear() method as noexcept to match the sibling
DynSlist::clear(); specifically update the documentation block for clear()
(which currently references empty()) by deleting the "@param none" entry and
change the method signature void clear() { empty(); } to a noexcept version
(e.g., void clear() noexcept { empty(); }) so the declaration and docs reflect
that it cannot throw.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 06c76c7c-e033-4e64-848d-761cdb2546a6
📒 Files selected for processing (5)
graph-dry.Hhtlist.Htpl_array.Htpl_dynDlist.Htpl_dyn_slist_nc.H
🚧 Files skipped from review as they are similar to previous changes (4)
- tpl_dynDlist.H
- tpl_array.H
- graph-dry.H
- htlist.H
- Elimina anotaciones redundantes '@param none' y '@return void' en métodos clear() de múltiples contenedores (tpl_arrayStack.H, tpl_dynListStack.H, tpl_dynArray.H, archeap.H, hashDry.H, tpl_arrayQueue.H, tpl_array.H, tpl_hash.H, tpl_dynSetHash.H, tpl_graph.H, htlist.H, tpl_lhash.H, tpl_dynSetTree.H, tpl_dyn_slist_nc.H, tpl_linHash.H, tpl_dynListQueue.H, tpl_dynBinHeap.H, ringfilecache.H, tpl_dynSkipList.H, tpl_memArray.H, tpl_dynDlist.H). - Corrige 'searching criteria' a 'searching criterion' en ah-dry.H para consistencia gramatical. - Añade métodos const get_next() y get_data() en slink_nc.H y tpl_snode_nc.H. - Corrige formato LaTeX en tpl_polynomial.H y README.md. - Añade pruebas de casos límite en Tests/quadtree_test.cc. - Mejora documentación de graph-dry.H eliminando '@param none' innecesario. - Corrige enlaces y formato en README.md y README.es.md.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tpl_dyn_slist_nc.H (1)
74-123:⚠️ Potential issue | 🟡 MinorShould fix: complete the Doxygen for the newly exposed public methods.
Several changed public methods still do not match the repository's documentation contract. Examples:
empty()/clear()omit@return, the queue/stack helpers omit applicable@return/@throwstags,operator[]omits@throws, and the updated accessor blocks still use@throwinstead of the repo's required@throws.Based on learnings, “All public member functions must have
brief,param,return, andthrowsDoxygen tags” and “No Doxygen warnings for changed files in code review.”Also applies to: 134-182, 195-236, 303-325
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tpl_dyn_slist_nc.H` around lines 74 - 123, Public method Doxygen is incomplete/inconsistent: update the docs for empty(), clear(), insert(), append(), operator[] and the accessor blocks to include `@brief`, `@param` (where applicable), `@return` (for functions returning a value or reference) and `@throws` (use `@throws` everywhere — replace any `@throw`) to match the repo contract; specifically add an explicit "@return void" or appropriate "@return reference to ..." for empty()/clear()/insert()/append()/operator[] and add "@throws std::bad_alloc" (or the correct exception) where allocations or bounds errors may occur, and ensure accessor blocks use `@throws` not `@throw` so no Doxygen warnings are produced (look for symbols empty, clear, insert, append, operator[], and the accessor methods mentioned).tpl_snode_nc.H (1)
62-93:⚠️ Potential issue | 🟡 MinorShould fix — finish Doxygen on these const overloads and the updated iterator accessor.
The API additions themselves look fine, but this changed public surface still does not meet the header documentation contract: the new const overloads are missing the complexity/precondition notes, and
Iterator::get_curr()still only has a brief inline comment.As per coding guidelines, "All public member functions must have
@brief,@param,@return, and@throwsDoxygen tags", "Include time/space complexity in function documentation as@noteor in description", and "Document preconditions (@pre) and postconditions (@post) when relevant".Also applies to: 118-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tpl_snode_nc.H` around lines 62 - 93, Update the missing Doxygen for the newly added const overloads and the iterator accessor: add full Doxygen blocks for const T & get_data() const, const Snode_Nc * get_next() const, Snode_Nc * remove_next(), Snode_Nc *& get_next(), and Iterator::get_curr() that include `@brief`, `@return`, and `@throws` (even if "none"); add `@note` with time/space complexity (e.g., O(1) for pointer ops) and include `@pre/`@post where relevant (e.g., precondition that next exists for remove_next(), postcondition about returned pointer and list linkage), keeping wording consistent with the project's doc style.
♻️ Duplicate comments (1)
ringfilecache.H (1)
870-875:⚠️ Potential issue | 🟡 MinorComplete the Doxygen contract for
clear()by adding@return.Line 870 introduces docs for a new public API, but the block still misses
@return, which is required by this repository’s public-method doc contract.Proposed doc patch
/** `@brief` Removes all entries from the cache. + `@return` void `@throws` none `@note` This method is an alias for empty() and is noexcept. */As per coding guidelines, "All public member functions must have
@brief,@param,@return, and@throwsDoxygen tags."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ringfilecache.H` around lines 870 - 875, The Doxygen for the public method clear() is missing a `@return` tag; update the documentation for clear() (the public noexcept alias of empty()) to include an `@return` describing what callers receive (e.g., "void" or "Nothing; the cache will be empty after the call") consistent with the repo contract, and keep existing `@brief` and `@throws` entries and the note that it is an alias for empty() and is noexcept.
🧹 Nitpick comments (1)
Tests/quadtree_test.cc (1)
194-204: MakeClearYieldsSameStateAsEmptycompareclear()vsempty()explicitly.Line 194 names an equivalence claim, but the body only verifies lookup misses after
clear(). Add side-by-side assertions against a tree cleared withempty().Based on learnings: "Tests must verify algorithmic correctness, not just absence of crashes."♻️ Suggested test tightening
TEST(QuadTreeBasic, ClearYieldsSameStateAsEmpty) { - QuadTree tree(0, 100, 0, 100, 4); - tree.insert(Point(25, 25)); - tree.insert(Point(75, 75)); + QuadTree tree_clear(0, 100, 0, 100, 4); + QuadTree tree_empty(0, 100, 0, 100, 4); + tree_clear.insert(Point(25, 25)); + tree_clear.insert(Point(75, 75)); + tree_empty.insert(Point(25, 25)); + tree_empty.insert(Point(75, 75)); - tree.clear(); + tree_clear.clear(); + tree_empty.empty(); - EXPECT_EQ(tree.search(Point(25, 25)), nullptr); - EXPECT_EQ(tree.search(Point(75, 75)), nullptr); + EXPECT_EQ(tree_clear.search(Point(25, 25)), tree_empty.search(Point(25, 25))); + EXPECT_EQ(tree_clear.search(Point(75, 75)), tree_empty.search(Point(75, 75))); + EXPECT_NE(tree_clear.get_root(), nullptr); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/quadtree_test.cc` around lines 194 - 204, The test QuadTreeBasic::ClearYieldsSameStateAsEmpty only checks that search returns nullptr after tree.clear(), but doesn't verify that the cleared tree's state equals a freshly empty tree; modify the test to create a second QuadTree constructed the same way and call its empty() (or ensure it's in the empty state), then assert that both trees behave identically: compare that search(Point(25,25)) and search(Point(75,75)) are nullptr for both trees and, if available, compare any public state/query results (e.g. size(), isEmpty(), or bounding-node structure) between the tree after clear() and the freshly empty QuadTree to ensure clear() yields the same state as empty(); use the QuadTree constructor, clear(), empty(), and search() identifiers to locate where to add the side-by-side assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ah-dry.H`:
- Around line 543-549: Update the Doxygen throws contract for the membership
APIs: remove the incorrect "@throws none" and instead document that contains_if
may propagate exceptions thrown by the predicate (when the predicate is not
noexcept) and contains may propagate exceptions from operator==; apply the same
change to the related comment block for contains (the later block around lines
557-562) and use the function names contains_if and contains in the text so the
intent is clear.
- Around line 551-555: The const contains_if currently forwards its Operation
via std::forward which can select the non-const find_ptr and permit mutation;
fix it by forcing the const overload of find_ptr: change the const contains_if
to pass a const reference to the predicate (e.g. accept or cast to const
Operation& and call find_ptr(static_cast<const Operation&>(operation)) or simply
use std::as_const(operation)) instead of std::forward, so find_ptr's const path
is used and predicates cannot mutate the container.
In `@htlist.H`:
- Around line 1407-1411: The Doxygen for the new public member function clear()
is missing an `@return` tag; update the comment block for clear() to include
"@return void" (or equivalent text stating it returns nothing) while keeping the
existing `@brief` and "@throws none" lines and omitting any `@param` (since clear()
has no parameters) so the public API doc meets the requirement that all public
member functions include `@brief`, `@param`, `@return`, and `@throws` tags.
In `@slink_nc.H`:
- Around line 103-111: Update the public API documentation for the
iterator/accessor methods (get_next(), get_curr_ne(), get_curr(), next()) by
replacing terse inline comments with full Doxygen blocks that include `@brief`,
`@param` (if any), `@return`, `@throws` (use "none" when nothing is thrown), and add
complexity notes (`@note`) for time/space, plus `@pre/`@post conditions where
relevant (e.g., pointer non-null or iterator validity). Ensure get_next()'s
contract documents that it returns a const pointer to the next Slink_Nc,
asserts/non-null precondition, O(1) time, no exceptions, and any postconditions;
apply the same style and level of detail to get_curr_ne(), get_curr(), and
next() so all public members meet the project's required tags and wording
conventions.
In `@tpl_array.H`:
- Around line 336-340: Update the Doxygen block for the public method clear() to
include the missing required tags: add an `@param` none (or `@param`[in] none) entry
to denote there are no parameters, add an `@return` void line to document the
return type, and add `@throws` none to explicitly state no exceptions are thrown;
keep the existing `@brief` and `@see` empty() lines intact so the clear()
documentation satisfies the project's rule that all public members include
`@brief`, `@param`, `@return`, and `@throws`.
- Around line 336-341: The new public method clear() lacks unit tests and its
Doxygen is missing the required `@return` tag; add "@return void" to the clear()
Doxygen block (keeping `@brief` and `@throws` none) and add unit tests for clear()
that exercise: call clear() on a populated container and assert size() == 0 and
is_empty() == true, call clear() on an already empty container and assert it is
safe (no throw) and still size() == 0 and is_empty() == true, and include any
necessary setup/teardown to create a non-empty instance before clearing;
reference the clear(), size(), and is_empty() members when adding tests.
In `@tpl_dyn_slist_nc.H`:
- Around line 47-55: The Doxygen for Dyn_Slist_Nc<T> currently only names the
type parameter; update the class comment to document the semantic requirements
on T: state that T must be Destructible and CopyConstructible (or generally
Copyable) because operations like insert(), append(), copy
construction/assignment of the container, and element destruction rely on those
properties; mention any exceptions (e.g. noexcept or move-only not supported)
and where relevant reference the class name Dyn_Slist_Nc<T> and the member
functions insert() and append() so readers can find the constraints.
- Around line 319-325: The const operator[] implementation must explicitly check
bounds before returning; in operator[] const (and mirror in the non-const
overload) verify that the requested index n is within range by walking the
Iterator or checking has_curr and then call ah_range_error_if(condition) when n
>= num_items (or when the iterator runs out) so you raise a deterministic range
error instead of unconditionally returning it.get_curr(); update the code around
Iterator it(...), the loop, and the final return to perform this check and
invoke ah_range_error_if(...) on out-of-range before calling it.get_curr().
In `@tpl_dynDlist.H`:
- Around line 164-170: The Doxygen block for the public method clear() is
missing mandatory tags; update the comment for clear() (which calls empty()) to
include `@return` describing the state after call (e.g., "void; container will be
empty") and an `@throws` tag (e.g., "none" or list exceptions if empty() can
throw) so it complies with the repo rule requiring `@brief`, `@param`, `@return`, and
`@throws` for all public members; reference the clear() method and its aliasing to
empty() to ensure the `@throws` text matches empty()'s exception guarantees.
In `@tpl_memArray.H`:
- Around line 346-359: Add explicit `@return` Doxygen tags to the two public
methods empty() and clear(): update the empty() comment block to include
"@return void" (or equivalent text) and update the clear() comment block to
include "@return void" as well, keeping existing `@brief` and `@throws` lines and
the `@see` in clear(); ensure tags follow the repository's Doxygen style for
public member functions.
- Around line 354-359: Add a GTest for the new public MemArray::clear() that
verifies its contract: create a MemArray<T>, populate it (or reserve and push
elements), record pre-clear capacity/allocated state (e.g., capacity() or
pointer/data presence), call clear(), then assert MemArray::empty() is true and
size()==0 while capacity (or allocation pointer) is preserved (unchanged or
non-null as expected); also include a test case where clear() is noexcept (call
in noexcept context or static_assert if applicable) and one that clears an
already-empty MemArray to cover the pre/postconditions. Use the MemArray
constructor, clear(), empty(), size(), capacity() and any
data()/buffer-identifying accessors from the class to make the assertions.
---
Outside diff comments:
In `@tpl_dyn_slist_nc.H`:
- Around line 74-123: Public method Doxygen is incomplete/inconsistent: update
the docs for empty(), clear(), insert(), append(), operator[] and the accessor
blocks to include `@brief`, `@param` (where applicable), `@return` (for functions
returning a value or reference) and `@throws` (use `@throws` everywhere — replace
any `@throw`) to match the repo contract; specifically add an explicit "@return
void" or appropriate "@return reference to ..." for
empty()/clear()/insert()/append()/operator[] and add "@throws std::bad_alloc"
(or the correct exception) where allocations or bounds errors may occur, and
ensure accessor blocks use `@throws` not `@throw` so no Doxygen warnings are
produced (look for symbols empty, clear, insert, append, operator[], and the
accessor methods mentioned).
In `@tpl_snode_nc.H`:
- Around line 62-93: Update the missing Doxygen for the newly added const
overloads and the iterator accessor: add full Doxygen blocks for const T &
get_data() const, const Snode_Nc * get_next() const, Snode_Nc * remove_next(),
Snode_Nc *& get_next(), and Iterator::get_curr() that include `@brief`, `@return`,
and `@throws` (even if "none"); add `@note` with time/space complexity (e.g., O(1)
for pointer ops) and include `@pre/`@post where relevant (e.g., precondition that
next exists for remove_next(), postcondition about returned pointer and list
linkage), keeping wording consistent with the project's doc style.
---
Duplicate comments:
In `@ringfilecache.H`:
- Around line 870-875: The Doxygen for the public method clear() is missing a
`@return` tag; update the documentation for clear() (the public noexcept alias of
empty()) to include an `@return` describing what callers receive (e.g., "void" or
"Nothing; the cache will be empty after the call") consistent with the repo
contract, and keep existing `@brief` and `@throws` entries and the note that it is
an alias for empty() and is noexcept.
---
Nitpick comments:
In `@Tests/quadtree_test.cc`:
- Around line 194-204: The test QuadTreeBasic::ClearYieldsSameStateAsEmpty only
checks that search returns nullptr after tree.clear(), but doesn't verify that
the cleared tree's state equals a freshly empty tree; modify the test to create
a second QuadTree constructed the same way and call its empty() (or ensure it's
in the empty state), then assert that both trees behave identically: compare
that search(Point(25,25)) and search(Point(75,75)) are nullptr for both trees
and, if available, compare any public state/query results (e.g. size(),
isEmpty(), or bounding-node structure) between the tree after clear() and the
freshly empty QuadTree to ensure clear() yields the same state as empty(); use
the QuadTree constructor, clear(), empty(), and search() identifiers to locate
where to add the side-by-side assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bc651780-b01f-400d-a5c5-d0ccccc69d70
📒 Files selected for processing (29)
README.es.mdREADME.mdTests/quadtree_test.ccah-dry.Harcheap.Hgraph-dry.HhashDry.Hhtlist.Hringfilecache.Hslink_nc.Htpl_array.Htpl_arrayQueue.Htpl_arrayStack.Htpl_dynArray.Htpl_dynBinHeap.Htpl_dynDlist.Htpl_dynListQueue.Htpl_dynListStack.Htpl_dynSetHash.Htpl_dynSetTree.Htpl_dynSkipList.Htpl_dyn_slist_nc.Htpl_graph.Htpl_hash.Htpl_lhash.Htpl_linHash.Htpl_memArray.Htpl_polynomial.Htpl_snode_nc.H
✅ Files skipped from review due to trivial changes (8)
- tpl_polynomial.H
- tpl_dynBinHeap.H
- archeap.H
- graph-dry.H
- tpl_dynListQueue.H
- tpl_dynSkipList.H
- README.es.md
- README.md
🚧 Files skipped from review as they are similar to previous changes (9)
- tpl_dynListStack.H
- tpl_arrayQueue.H
- tpl_lhash.H
- tpl_arrayStack.H
- tpl_linHash.H
- hashDry.H
- tpl_dynSetTree.H
- tpl_hash.H
- tpl_dynSetHash.H
- Formatea código para consistencia de espacios y alineación - Añade documentación faltante en métodos clear() y empty() - Corrige firmas de métodos para aceptar SearchLimits por referencia constante - Añade chequeo de límites en operador [] de Dyn_Slist_Nc - Actualiza especificaciones de excepciones en ah-dry.H - Mejora claridad de comentarios en slink_nc.H y tpl_snode_nc.H - Añade prueba unitaria para método clear() en MemArray
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tpl_dyn_slist_nc.H (1)
93-98:noexceptconsistency:clear()delegates to non-noexceptempty().
clear()is declarednoexceptbut callsempty()which lacks thenoexceptspecifier. If any operation inempty()throws (e.g., a non-conforming destructor),std::terminate()will be called.For consistency, either mark
empty()asnoexceptas well (since its documentation already states@throws none), or removenoexceptfromclear().Suggested fix: mark empty() as noexcept
/** `@brief` Empties the container. `@throws` none */ - void empty() + void empty() noexcept {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tpl_dyn_slist_nc.H` around lines 93 - 98, clear() is declared noexcept but calls empty() which is not noexcept; update the declaration and definition of empty() to add the noexcept specifier (since its docs say "@throws none") so both functions are consistently noexcept, or alternatively remove noexcept from clear() if empty() cannot be guaranteed non-throwing—prefer adding noexcept to empty() (function name: empty(), caller: clear()) and rebuild to ensure no compiler errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tpl_dyn_slist_nc.H`:
- Around line 313-335: The loop variable in both operator[] and its const
version uses a signed int (int i) while comparing to a size_t n, risking
signed/unsigned mismatch and overflow; change the loop counter to an unsigned
type matching n (e.g., size_t i) in the functions operator[](size_t const& n)
and the const overload (where Iterator it is constructed), so the for loops use
size_t i = 0; i < n; ++i, it.next_ne() to avoid undefined behavior for large
indices.
In `@tpl_dynDlist.H`:
- Around line 156-162: The Doxygen wrongly claims "This is an alias of empty()";
update the comment for the method (replace that line) to describe the actual
behavior — e.g., state that the function removes all elements from the
container, leaves it empty (size() == 0), and resets internal state; if the
intent was to note equivalence to empty() mention "performs the same result as
empty()" rather than "is an alias of empty()". Ensure references to empty()
remain accurate in the comment for clarity.
---
Nitpick comments:
In `@tpl_dyn_slist_nc.H`:
- Around line 93-98: clear() is declared noexcept but calls empty() which is not
noexcept; update the declaration and definition of empty() to add the noexcept
specifier (since its docs say "@throws none") so both functions are consistently
noexcept, or alternatively remove noexcept from clear() if empty() cannot be
guaranteed non-throwing—prefer adding noexcept to empty() (function name:
empty(), caller: clear()) and rebuild to ensure no compiler errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d6367c2a-2ede-40fd-8d39-ce9c101bf567
📒 Files selected for processing (17)
Alpha_Beta.HBacktracking.HBranch_And_Bound.HExamples/adversarial_artificial_example.ccNegamax.HState_Search_IDA_Star.HTests/memarray.ccah-dry.Hhtlist.Hsearch_move_ordering.Hslink_nc.Htpl_array.Htpl_arrayQueue.Htpl_dynDlist.Htpl_dyn_slist_nc.Htpl_memArray.Htpl_snode_nc.H
✅ Files skipped from review due to trivial changes (2)
- Tests/memarray.cc
- tpl_array.H
🚧 Files skipped from review as they are similar to previous changes (4)
- slink_nc.H
- tpl_snode_nc.H
- tpl_memArray.H
- tpl_arrayQueue.H
Se cambia la condición del bucle en el constructor de copia de tpl_memArray.H de `i < dim` a `i < n` para copiar el número correcto de elementos. refactor: Simplifica llamadas y corrige tipos en contenedores - En ah-dry.H, se elimina `std::as_const` innecesario en `contains_if`. - En tpl_dyn_slist_nc.H, se cambia el tipo del índice en bucles de `int` a `size_t`. - En tpl_array.H y htlist.H, se eliminan comentarios redundantes `@return void`. - En tpl_dynSetHash.H, se reemplaza `this->HashTable<Key, Cmp>::contains(key)` por `Base::contains(key)`. - En tpl_dynDlist.H, se mejora la documentación de `empty()` y se simplifica `clear()`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests
Documentation
Chores