Skip to content

Partial fix for #13747 assertion in Token::update_property_info() (no variable declaration in static_assert) #7462

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

chrchr-github
Copy link
Collaborator

Or maybe fIsName should not be set?

@chrchr-github chrchr-github marked this pull request as draft April 13, 2025 12:30
@chrchr-github chrchr-github reopened this Apr 14, 2025
@chrchr-github chrchr-github changed the title Partial fix for #13747 assertion in Token::update_property_info() (%type% matches true|false) Partial fix for #13747 assertion in Token::update_property_info() (no variable declaration in static_assert) Apr 14, 2025
@chrchr-github chrchr-github marked this pull request as ready for review April 14, 2025 07:40
lib/tokenize.cpp Outdated
@@ -4708,6 +4708,10 @@ void Tokenizer::setVarIdPass1()

if (!isC() && Token::simpleMatch(tok2, "const new"))
continue;
if (!isC() && Token::simpleMatch(tok, "static_assert (")) {
Copy link
Owner

Choose a reason for hiding this comment

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

I didn't look carefully but this feels rather hacky to me. So we would still get such assertions for:

something(true && false);

spontanously true should not be seen as a valid variable type and false should not be seen as a valid variable name if it's a c++ file.. but I assume you looked into that already?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This something(true && false); seems to work as expected in an executable scope, but of course static_assert can appear in global or other scopes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

C23+ also has static_assert.

Copy link
Owner

@danmar danmar Apr 17, 2025

Choose a reason for hiding this comment

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

And you can achieve a static assertion with a macro. I have seen such:

#define STATIC_ASSERT(X)    char  dummy  [ (X) ? 1 : -1 ]

This can be used in the global scope

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My comment about the global scope was intended as a justification why static_assert needs special handling.

Copy link
Owner

@danmar danmar Apr 17, 2025

Choose a reason for hiding this comment

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

yeah .. I wanted to point out it would also be good to cover STATIC_ASSERT etc..

so such testcase would be good imho:

    const char code[] = "STATIC_ASSERT(true && false);\n";
    const char expected[] = "1: STATIC_ASSERT ( true && false ) ;\n";
    ASSERT_EQUALS(expected, tokenize(code));

Copy link
Owner

@danmar danmar Apr 17, 2025

Choose a reason for hiding this comment

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

my first question was if you looked into if true and false can be handled more intelligently instead? It does not seem impossible to determine that true && false is not a valid C++ variable declaration.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

my first question was if you looked into if true and false can be handled more intelligently instead? It does not seem impossible to determine that true && false is not a valid C++ variable declaration.

This is, among other things, tied to %type% matching true|false. I didn't want to touch that in this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah .. I wanted to point out it would also be good to cover STATIC_ASSERT etc..

so such testcase would be good imho:

I believe this currently leads to an internalError. But there were no such errors in daca when I last checked, so code like that seems to be uncommon in C++.

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.

5 participants