Repository navigation
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3949 +/- ##
=======================================
Coverage 68.67% 68.68%
=======================================
Files 714 716 +2
Lines 115483 115555 +72
=======================================
+ Hits 79311 79367 +56
- Misses 36172 36188 +16 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
| :param sl: Pointer to the SectionListIterator. | ||
| :returns: Pointer to the next Section. | ||
| :returns: Pointer to the next Section, or NULL if only deleted sections were left. |
There was a problem hiding this comment.
what if there were never sections in the list? Would it be more accurate to say "or NULL if no active sections are left"?
In any case, I think this should be in the present tense.
There was a problem hiding this comment.
Yes, that's more accurate: it returns NULL whenever it reaches the end, including an empty list, so I used your wording in the present tense (45d48f3).
nrn_sectionlist_iterator_done does not skip deleted sections, so with only deleted sections left it returns 0 and next returns NULL.
| @@ -2000,6 +2009,9 @@ Miscellaneous | |||
| SectionListIterator* iter = nrn_sectionlist_iterator_new(section_list); | |||
| while (!nrn_sectionlist_iterator_done(iter)) { | |||
There was a problem hiding this comment.
Do we still need the nrn_sectionlist_iterator_done call? It's not great if we have to do TWO checks for valid sections
There was a problem hiding this comment.
No, it isn't needed. nrn_sectionlist_iterator_next returns NULL at the end of the list too, not only when the sections left are deleted, so checking its result is enough. 168ef64 changes the nrn_allsec example to that loop and drops the example under nrn_sectionlist_iterator_done, which now says a loop doesn't need it (it stays for existing code). The api tests use the same loop now, and sectionlist_iterator_deleted.cpp also checks that done is non-zero once next returns NULL.
While checking this I found that a loop must not delete or remove a section it hasn't reached yet. In nrn_allsec()'s list delete_section frees the item right away, and SectionList.remove() does the same in a SectionList, so if that section is the next one the iterator reads freed memory (valgrind shows it). That was already true with the old loop. a427371 says so in the docs. Deleting or removing the section just returned is fine.
nrn_sectionlist_iterator_next returns NULL at the end of the list as well as when only deleted sections are left, so the examples check only its result. The iterator test uses that loop and checks that nrn_sectionlist_iterator_done is non-zero once next returns NULL.
… tests Deleting a section during an nrn_allsec() loop frees its list item at once, and SectionList.remove() frees one in a SectionList, so a loop must not delete or remove a section it has not reached yet (valgrind shows the iterator reading the freed item). The section just returned is fine. The skip-and-drop sentence applies to SectionLists. sections.cpp and sectionlist_to_array.cpp use the one-check loop too.
| break; // only deleted sections were left | ||
| } | ||
| Section* sec; | ||
| while ((sec = nrn_sectionlist_iterator_next(iter))) { |
There was a problem hiding this comment.
one more set of parentheses than needed
There was a problem hiding this comment.
The inner pair was there because gcc -Wall warns about a bare assignment as the condition ("suggest parentheses around assignment used as truth value"). a81be98 compares with NULL instead, which needs no extra pair.
| // Process Section | ||
| } | ||
| nrn_sectionlist_iterator_free(iter); | ||
| It does not look past deleted sections, so when only deleted sections are |
There was a problem hiding this comment.
so is this still useful? I don't think we can get rid of it, unfortunately, but I think we can have the docs point to the way to loop without doing this
There was a problem hiding this comment.
It isn't needed for a loop, but it stays for existing code. a81be98 starts its docs with a note that points to the one-check loop in nrn_allsec.
… need it Lead its docs with a note that points to the one-check loop in nrn_allsec. The nrn_allsec example compares with NULL instead of doubling the parentheses (gcc -Wall warns about a bare assignment as the condition).
|
Follow-up to #3947, which merged before I pushed this.
nrn_sectionlist_iterator_nextnow drops a deleted section's item and the list's reference to it. If that was the last reference theSectionis freed, so a caller that keeps aSection*across passes needsnrn_section_ref. It can also return NULL when only deleted sections were left, which the test already handles but the docs didn't say.Since
nrn_sectionlist_iterator_nextreturns NULL at the end of the list as well, the examples now loop on its result alone, and the docs saynrn_sectionlist_iterator_doneisn't needed for a loop (it stays for existing code). The api tests use that loop too. The docs also say a loop must not delete or remove a section it hasn't reached yet: innrn_allsec()'s listdelete_sectionfrees the item right away, andSectionList.remove()frees it in a SectionList, so the iterator can read freed memory. That was already the case before.