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 #13615: False positive: Misra C 17.3: UINT32_C #7276

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

Conversation

swasti16
Copy link
Contributor

@swasti16 swasti16 commented Feb 3, 2025

No description provided.

@firewave
Copy link
Collaborator

firewave commented Feb 3, 2025

FYI We used to replace these macros but they were removed a while ago and need to be re-added.

@danmar
Copy link
Owner

danmar commented Feb 4, 2025

FYI We used to replace these macros but they were removed a while ago and need to be re-added.

@firewave we need to keep UINT32_C in some way for misra. For instance the code UINT32_C(10UL) is non-compliant according to misra 7.5. The code 10UL does not violate 7.5.

The Misra checker for rule 7.5 could in theory use the "raw tokens" instead but that means that violations in headers are not detected and there could also be false negatives when macros is used.

So for the sake of Misra I would suggest that we don't replace UINT32_C. Or do you think there is some better replacement that will still mean we can detect misra violations properly?

@firewave
Copy link
Collaborator

firewave commented Feb 4, 2025

@firewave we need to keep UINT32_C in some way for misra. For instance the code UINT32_C(10UL) is non-compliant according to misra 7.5. The code 10UL does not violate 7.5.

Isn't that what the macro information is for? This seems to be similar to #6559.

@danmar
Copy link
Owner

danmar commented Feb 4, 2025

I am saying that if we would add something like this:

 <define name="UINT32_C(VALUE)" value="VALUE"/>

Then the dumpfile will not contain the necessary information. If the code is UINT32_C(10UL) then that would be preprocessed to 10UL which is compliant according to 7.5 and Token::macroName will not say UINT32_C anywhere because no token comes from that macro.

@firewave
Copy link
Collaborator

firewave commented Feb 4, 2025

Right because those are just replacements and are not treated as macros. Maybe that should be changed.

But as you noted in the past UINT32_C needs to be implemented via the platform so in that case we should have the information, right?

@danmar
Copy link
Owner

danmar commented Feb 4, 2025

But as you noted in the past UINT32_C needs to be implemented via the platform so in that case we should have the information, right?

I am not sure how to implement that.

How about this define:

  <define name="UINT32_C(V)" value="(uint32_t)(V)"/>

If the code says UINT32_C(10UL) then the preprocessor output will be (uint32_t)(10UL) and for the "(" token the Token::macroName will be "UINT32_C"..

@firewave
Copy link
Collaborator

firewave commented Feb 4, 2025

I am not sure how to implement that.

It should no different from the existing defines we already generate.

But these should be flagged as non-internal ones. That is a feature we need to implement in simplecpp first. And it is probably not important.

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