Skip to content

Fix #12910 FP containerOutOfBounds with overloaded template function #7453

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 16, 2025

Conversation

chrchr-github
Copy link
Collaborator

Feels hacky though.

if (matches.size() == 1)
if (matches.size() == 1 && std::none_of(functionList.begin(), functionList.end(), [tok](const Function& f) {
const auto nTok = tok->str().size();
return startsWith(f.name(), tok->str()) && f.name().size() > nTok + 2 && f.name()[nTok + 1] == '<';
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
return startsWith(f.name(), tok->str()) && f.name().size() > nTok + 2 && f.name()[nTok + 1] == '<';
return startsWith(f.name(), tok->str() + "<") && f.name().size() > nTok + 2;

I still feel it's hacky but this seems better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The function name seems to be func < arg > instead of func<arg> for some reason.

Copy link
Owner

@danmar danmar Apr 14, 2025

Choose a reason for hiding this comment

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

yeah there is (sometimes?) space before the "<" . we did not used to have it. maybe the extra spaces are removed in the future again who knows.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you have examples for either case I will see if I can bisect this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you have examples for either case I will see if I can bisect this.

The spaces appear in the output of --debug -v (e.g. name:), but I don't know what the significance is.

Copy link
Collaborator

@firewave firewave Apr 14, 2025

Choose a reason for hiding this comment

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

template <class T> void fun()
{
}

int main()
{
    fun<int>();
}

1.77

1:
|
4:
5: int main ( )
6: {
7: fun<int> ( ) ;
8: } void fun<int> ( )
2: {
3: }
[...]
Scope: 000001F7E6853E30 Function
    className: fun<int>
[...]
##AST
( 'signed int'
`-main

( 'void'
`-fun<int>

( 'void'
`-fun<int>

1.78

1:
|
4:
5: int main ( )
6: {
7: fun < int > ( ) ;
8: } void fun < int > ( )
2: {
3: }
[...]
Scope: 000001CBB41650D0 Function
    className: fun < int >
[...]
##AST
( 'signed int'
`-main

( 'void'
`-fun < int >

( 'void'
`-fun < int >

1.82

1:
|
4:
5: int main ( )
6: {
7: fun<int> ( ) ;
8: } void fun<int> ( )
2: {
3: }
[...]
Scope: 000001E34CC268D0 Function
    className: fun < int >
[...]
##AST
( 'signed int'
`-main

( 'void'
`-fun < int >

( 'void'
`-fun < int >

Copy link
Collaborator

Choose a reason for hiding this comment

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

The 1.78 change has been bisected to 0943b21.

The 1.82 change has been bisected to eaadfb3.

If we make any changes we should add unit and Python tests for this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Something in here we need to file a ticket about?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The inconsistent use of spaces in template names, perhaps.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@danmar
Copy link
Owner

danmar commented Apr 13, 2025

Feels hacky though.

I agree.. it feels like there could be cases that does not work as wanted.

Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

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

well I can accept this. If the space is removed in the future we'll have to change this.

@chrchr-github chrchr-github merged commit f837929 into danmar:main Apr 16, 2025
53 checks passed
@chrchr-github chrchr-github deleted the chr_12910 branch April 16, 2025 17:27
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