Skip to content

Handle explicit template specializations in sg.pl#134

Merged
wdeconinck merged 4 commits intodevelopfrom
fix/sg_pl
Mar 25, 2026
Merged

Handle explicit template specializations in sg.pl#134
wdeconinck merged 4 commits intodevelopfrom
fix/sg_pl

Conversation

@Ozaq
Copy link
Copy Markdown
Member

@Ozaq Ozaq commented Feb 26, 2026

Description

sg.pl crashes when a header contains an explicit template specialization (template <>) because template_args() expects class/bool/int after '<' but gets '>'.

Return an empty parameter list from template_args() on 'template <>' and skip the entire specialization in parse_template(), since explicit specializations are not new class definitions and should not generate .b files.

Contributor Declaration

By opening this pull request, I affirm the following:

  • All authors agree to the Contributor License Agreement.
  • The code follows the project's coding standards.
  • I have performed self-review and added comments where needed.
  • I have added or updated tests to verify that my changes are effective and functional.
  • I have run all existing tests and confirmed they pass.

Copy link
Copy Markdown

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

Prevents sg.pl from crashing on explicit template specializations (template <>) by detecting the empty parameter list case and skipping parsing of the specialization body.

Changes:

  • Return an empty parameter list from template_args() when encountering template <>.
  • In parse_template(), detect explicit specializations and skip the entire declaration/body so they don’t generate .b outputs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread cmake/sg.pl
Comment thread cmake/sg.pl
sg.pl crashes when a header contains an explicit template
specialization (template <>) because template_args() expects
class/bool/int after '<' but gets '>'.

Return an empty parameter list from template_args() on 'template <>'
and skip the entire specialization in parse_template(), since explicit
specializations are not new class definitions and should not generate
.b files.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@wdeconinck
Copy link
Copy Markdown
Member

Hi @Ozaq could you please provide a test that was previously crashing?

@marcosbento
Copy link
Copy Markdown
Collaborator

marcosbento commented Feb 27, 2026

If I understood correctly, the issue occurs when the file contains something like:

// Forwarded classes
template <> class TheNuisance< std::string >;

class X {};

which ends with the failure > is not (class|bool|int) at cmake/sg.pl line 45. upon finding the <> of TheNuisance, where normally the whole construct would be ignored as it is just a class forward declaration.

Assuming this is indeed the problem the issue is fixed! But this certainly deserves a test to ensure the use case is handled properly

Copy link
Copy Markdown
Collaborator

@marcosbento marcosbento left a comment

Choose a reason for hiding this comment

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

Please add a test to cover the failure test that triggered these changes.

Comment thread cmake/sg.pl
Comment thread cmake/sg.pl Outdated
Copy link
Copy Markdown
Member

@wdeconinck wdeconinck left a comment

Choose a reason for hiding this comment

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

Test requested.

Verifies the fix from 5b88770 by running sg.pl on a header containing
both body and semicolon-only forms of template <>, asserting no crash,
correct .b file generation for regular/template classes, and proper
parser recovery for classes appearing after the specializations.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@Ozaq
Copy link
Copy Markdown
Member Author

Ozaq commented Mar 20, 2026

@wdeconinck / @marcosbento Tests added

@wdeconinck
Copy link
Copy Markdown
Member

All runners are failing

        Start 40: test_sg_pl
  Process not started
   /opt/actions-runner/work/_work/ecbuild/ecbuild/build/tests/sg_pl/run-test.sh
  [permission denied]
  33/40 Test #40: test_sg_pl .........................................***Not Run   0.00 sec

@marcosbento
Copy link
Copy Markdown
Collaborator

@wdeconinck I've fixed a few outstanding issues, and the ecbuild build (including the new test) is passing -- the issues detected downstream don't seem related.

Copy link
Copy Markdown
Member

@wdeconinck wdeconinck left a comment

Choose a reason for hiding this comment

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

Great Thanks All!

@wdeconinck wdeconinck merged commit 5ad6e00 into develop Mar 25, 2026
825 of 937 checks passed
@marcosbento marcosbento deleted the fix/sg_pl branch March 26, 2026 14:50
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.

4 participants