bugfix(worldbuilder): Check list size before accessing first element in ObjectOptions::_FindOrDont

[Caball009]

If you open the map Sand Serpent with the World Builder and click on the green object in the center of the blue circle, you get a debug assertion that accessing the front element of an empty list is not allowed. This PR fixes that.

[greptile-apps]

Greptile Overview

Greptile Summary

Fixed a debug assertion crash in World Builder when clicking on certain objects. The _FindOrDont function performed breadth-first search through a tree view but didn't check if the list was empty before calling front(), causing crashes when the tree was fully traversed.

• Added !itemsToEx.empty() check to while loop condition in both Generals and GeneralsMD versions
• Prevents undefined behavior when accessing front() on empty std::list
• Fix is minimal, targeted, and identical across both codebases

Confidence Score: 5/5

• This PR is safe to merge with no risk
• The fix is a standard defensive programming pattern that prevents undefined behavior. The change is minimal (adding !itemsToEx.empty() check), logically correct, and applied consistently to both codebases. The fix directly addresses the reported crash without introducing side effects.
• No files require special attention

Important Files Changed

Filename	Overview
Generals/Code/Tools/WorldBuilder/src/ObjectOptions.cpp	Added empty list check to prevent debug assertion when accessing front element
GeneralsMD/Code/Tools/WorldBuilder/src/ObjectOptions.cpp	Added empty list check to prevent debug assertion when accessing front element

_{Last reviewed commit: 2f5d827}

[greptile-apps]

_{2 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Additional Comments (2)

Generals/Code/Tools/WorldBuilder/src/ObjectOptions.cpp
GetChildItem can return NULL when there are no children. Should add null check before pushing to list to prevent processing invalid handles.

			HTREEITEM child = m_objectTreeView.GetChildItem(hItem);
			if (child) {
				itemsToEx.push_back(child);
			}

Prompt To Fix With AI

This is a comment left during a code review.
Path: Generals/Code/Tools/WorldBuilder/src/ObjectOptions.cpp
Line: 428:428

Comment:
`GetChildItem` can return NULL when there are no children. Should add null check before pushing to list to prevent processing invalid handles.

```suggestion
			HTREEITEM child = m_objectTreeView.GetChildItem(hItem);
			if (child) {
				itemsToEx.push_back(child);
			}
```

How can I resolve this? If you propose a fix, please make it concise.

---

GeneralsMD/Code/Tools/WorldBuilder/src/ObjectOptions.cpp
GetChildItem can return NULL when there are no children. Should add null check before pushing to list to prevent processing invalid handles.

			HTREEITEM child = m_objectTreeView.GetChildItem(hItem);
			if (child) {
				itemsToEx.push_back(child);
			}

Prompt To Fix With AI

This is a comment left during a code review.
Path: GeneralsMD/Code/Tools/WorldBuilder/src/ObjectOptions.cpp
Line: 428:428

Comment:
`GetChildItem` can return NULL when there are no children. Should add null check before pushing to list to prevent processing invalid handles.

```suggestion
			HTREEITEM child = m_objectTreeView.GetChildItem(hItem);
			if (child) {
				itemsToEx.push_back(child);
			}
```

How can I resolve this? If you propose a fix, please make it concise.

[xezon]

Is HTREEITEM a pointer?

Maybe just do while (!itemsToEx.empty())? Why test for front() here?

[Mauller]

I agree, only testing for empty should resolve this here, as when not empty you should always have a front iterator.

[Caball009]

It's not clear to me if the code can handle a nullptr as element of itemsToEx, so this seemed to me the safest option.

[Caball009]

Is HTREEITEM a pointer?

Yes. typedef struct _TREEITEM *HTREEITEM;

[xezon]

Is there any evidence that null is added into the container?

[Caball009]

https://learn.microsoft.com/en-us/cpp/mfc/reference/ctreectrl-class?view=msvc-170#getchilditem
https://learn.microsoft.com/en-us/cpp/mfc/reference/ctreectrl-class?view=msvc-170#getnextsiblingitem
can return a nullptr in theory. I don't know if that's possible in our use case.

[xezon]

Perhaps prevent it from adding null to the list? What would be the point of a null entry in the list? Would it do anything?

[Caball009]

Perhaps prevent it from adding null to the list?

That requires 2 checks, or 3 if we can't guarantee that the function parameter is never null.

What would be the point of a null entry in the list? Would it do anything?

I'm not sure and frankly I'm not sure it's worth my time to find out. I'd assume that replacing the MFC code is going to happen if we decide to revamp the World Builder.

[xezon]

I suggest add an assert for null in the loop, run World Builder, do whatever it takes to populate the list, then see if that assert ever hits. If it does not, then there is no null in the list and you can just keep the assert and remove the front test.