Skip to content
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

fix #13378: syntax error for guessed macro value #7218

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ludviggunne
Copy link
Contributor

@ludviggunne ludviggunne commented Jan 13, 2025

Example output:

Checking tickets/13378.c ...
Checking tickets/13378.c: START...
tickets/13378.c:3:5: error: literal used as function. Macro 'START' expands to '1'. Use -DSTART=... to specify a value, or -USTART to undefine it. [unknownMacro]
    START();
    ^

With -DSTART the syntax error remains:

Checking tickets/13378.c ...
Checking tickets/13378.c: START=1...
tickets/13378.c:3:5: error: syntax error [syntaxError]
    START();
    ^

@firewave
Copy link
Collaborator

You can specify actual macros via the CLI (at least with compilers): -D'name(args...)=definition'.

I used this recently somewhere in simplecpp or Cppcheck (most likely a local change) but I cannot remember where.

@ludviggunne
Copy link
Contributor Author

You can specify actual macros via the CLI (at least with compilers): -D'name(args...)=definition'.

I used this recently somewhere in simplecpp or Cppcheck (most likely a local change) but I cannot remember where.

Hm, sorry but could you point out how this affects this PR?

@firewave
Copy link
Collaborator

Use -DSTART=... to specify a value -> Use -DSTART(args...)=definition to specify it

And I guess having -USTART would only fix something if there was an #ifdef related to that macro. So maybe that should be dropped from the message.

This might also be addressed by fixing a missingInclude warning or providing a library.

On a side note - if we would apply that logic during the macro parsing we could (optionally) generate a configuration which already provides these - in some cases. That would require less user interaction. But that is obviously out of scope.

@ludviggunne
Copy link
Contributor Author

ludviggunne commented Jan 13, 2025

And I guess having -USTART would only fix something if there was an #ifdef related to that macro. So maybe that should be dropped from the message.

Ah, maybe I should have provided more context.
Here's the code from the ticket:

void foo(void) {
#ifdef START
    START();
#endif
}

@firewave
Copy link
Collaborator

Ah, maybe I should have provided more context.

My fault. I did not look at the ticket. But to be fair there are also no tests added.

But I guess you will also get that message if the preprocessor check is missing.

I will file tickets about my notes.

@ludviggunne
Copy link
Contributor Author

ludviggunne commented Jan 13, 2025

But to be fair there are also no tests added.

Oh yes, just thought it would be good to have it up as a draft to get som input on the messages.

@firewave
Copy link
Collaborator

Oh yes, just thought it would be good to have it up as a draft to get som input on the messages.

I think it is a good change we can built upon.

if (tok->strAt(1) == "(")
if (tok->strAt(1) == "(") {
if (tok->isExpandedMacro() && mSettings.userDefines.empty()) {
throw InternalError(tok, "literal used as function. Macro '" + tok->getMacroName() +
Copy link
Owner

Choose a reason for hiding this comment

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

my goal would be that this message is only written if the macro is defined by Preprocessor::getConfigs => that means the macro value is guessed by Cppcheck.

The configuration is passed to Tokenizer::simplifyTokens1. Can that be propagated here so that we can check if it contains the macro name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems the configuration is overriden by CLI/project defines by then already:

mCurrentConfig = mSettings.userDefines;

But that only happens if mSettings.userDefines is empty, so I think it should be enough to check the same condition in findGarbageCode.

@ludviggunne
Copy link
Contributor Author

This might also be addressed by fixing a missingInclude warning or providing a library.

That's a good point. I suppose it could make the message quite long, would it be appropriate to split it into multiple messages?

@firewave
Copy link
Collaborator

That's a good point. I suppose it could make the message quite long, would it be appropriate to split it into multiple messages?

I would keep it simple as it is not mention it at all. This is another case where the list of suggestions could be endless and misleading.

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