Skip to content

Conversation

bernardnormier
Copy link
Member

PHP does not support nesting a namespace with a namespace, but it supports sub-namespaces.

Prior to this PR, we were simply opening the namespace around each mapped PHP construct ... lots of namespaces. This PR reduces the number of PHP namespace reopening.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR improves namespace generation in PHP by consolidating namespace declarations to avoid unnecessary reopening. Instead of wrapping each PHP construct in its own namespace declaration, the code now uses a module-based approach with proper namespace stacking to handle nested modules while respecting PHP's lack of support for truly nested namespaces.

  • Replaces per-construct namespace wrapping with module-level namespace management
  • Implements namespace stack to handle module nesting by closing/reopening namespaces as needed
  • Removes redundant startNamespace/endNamespace calls from individual construct visitors

{
_out << eb;
_parentNamespaces.pop();
if (!_parentNamespaces.empty())
Copy link
Preview

Copilot AI Jul 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The stack could become empty during visitModuleEnd, but there's no check to prevent popping from an empty stack. This could cause undefined behavior if modules are not properly balanced.

Copilot uses AI. Check for mistakes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My assumption is the Parser guarantees proper balancing of start/end.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is guaranteed.

@bernardnormier bernardnormier requested review from pepone and removed request for externl August 1, 2025 13:32
Copy link
Member

@InsertCreativityHere InsertCreativityHere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@bernardnormier bernardnormier merged commit 978c790 into zeroc-ice:main Aug 1, 2025
27 checks passed
pepone pushed a commit to pepone/ice that referenced this pull request Sep 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants