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

refs #11928 / refs #10045 / fixes #11794 - generate limits.h/climits defines from platform #5414

Merged
merged 5 commits into from
Oct 21, 2023

Conversation

firewave
Copy link
Collaborator

@firewave firewave commented Sep 7, 2023

No description provided.

@firewave firewave force-pushed the platform-limits branch 9 times, most recently from ac3be5a to fa29f38 Compare September 8, 2023 16:35
@firewave firewave changed the title generate limits defines from platform refs #11928 / refs #10045 / fixes #11794 - generate limits defines from platform Oct 2, 2023
@firewave
Copy link
Collaborator Author

firewave commented Oct 2, 2023

The defines generated by defines.sh cannot be passed on to Cppcheck yet - I added https://trac.cppcheck.net/ticket/12028 about that.

The external scripts/logic should at some point be inlined into Cppcheck so you would only have to specify the compiler and no more other files. See https://trac.cppcheck.net/ticket/8956 about that.

@firewave
Copy link
Collaborator Author

firewave commented Oct 2, 2023

I don't know to to make the warnings in the added integerOverflow2 test appear. I somehow need to invoke the preprocessor first and I have no idea how to do that.

@firewave
Copy link
Collaborator Author

firewave commented Oct 4, 2023

I don't know to to make the warnings in the added integerOverflow2 test appear. I somehow need to invoke the preprocessor first and I have no idea how to do that.

@chrchr-github Any pointers on what to do here?

@chrchr-github
Copy link
Collaborator

I don't know to to make the warnings in the added integerOverflow2 test appear. I somehow need to invoke the preprocessor first and I have no idea how to do that.

@chrchr-github Any pointers on what to do here?

There is a checkP() function in TestOther. Can you call that?

@firewave
Copy link
Collaborator Author

firewave commented Oct 4, 2023

There is a checkP() function in TestOther. Can you call that?

That was obvious. I did not look at the code at all before. Unfortunately that once again uses custom preprocessing which does not match the production one.

That code is also redundant as hell so I will clean that up first.

@firewave firewave changed the title refs #11928 / refs #10045 / fixes #11794 - generate limits defines from platform refs #11928 / refs #10045 / fixes #11794 - generate limits.h/climits defines from platform Oct 5, 2023
@firewave firewave force-pushed the platform-limits branch 2 times, most recently from 92ddc99 to 9d2ef05 Compare October 11, 2023 09:31
@firewave firewave marked this pull request as ready for review October 11, 2023 09:31

int main(void)
{
PRINT_DEF(FLT_RADIX, d);
Copy link
Owner

Choose a reason for hiding this comment

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

What is the purpose of this? Is it the purpose that users will compile and run it? As far as I know this approach will not work in a avr controller for instance. Maybe an alternative would be to just preprocess a file with the target compiler.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What is the purpose of this?

It has multiple purposes:

  • for testing/development
  • for invoking via the add run_cppcheck.sh (not yet integrated / for developers/advanced user)
  • for invoking via the proposed --compiler= option (https://trac.cppcheck.net/ticket/8956) to generate a whole "native" configuration for the analysis instead of relying on a fixed platform configuration (basically integrating the aforementioned script)
    • this could also be used used to generate some kind of project file/fixed configuration which holds all these information which could be generated once in cases where the compiler or related development files are not available (the avr8 case you mentioned)

This is very much WIP and will probably stay like this for quite a while but we gotta start somewhere. 😀

Copy link
Owner

Choose a reason for hiding this comment

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

hmm I am a bit skeptic about merging this into the repo because of all the tools files.

If it's just something we are looking at in the near future and it will just be lying in the repo unused a year from now then it feels unfortunate.

The bash scripts are not portable.

And the solution for #8956 should be generic imho. It should be possible to use it to configure AVR/etc compiler defines.

How about breaking out the tools files into a separate PR ?

Copy link
Owner

@danmar danmar Oct 13, 2023

Choose a reason for hiding this comment

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

If the float.c is not executed.. only preprocessed. Then it can be used for embedded compilers also. My guess is that most compilers have a flag to only perform preprocessing.

Example usage would be:

<my-compiler> <preprocess-only flag> floats.c > compiler-defs.h
cppcheck --compiler-defs=compiler-defs.h ..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about breaking out the tools files into a separate PR ?

I would like to keep those together with the changes I did for the code and I based those on that output.

If it's just something we are looking at in the near future and it will just be lying in the repo unused a year from now then it feels unfortunate.

The bash scripts are not portable.

And the solution for #8956 should be generic imho. It should be possible to use it to configure AVR/etc compiler defines.

There's lots of unused and possibly broken stuff in the repo nobody has touched in years. Some of it is even being shipped in the official release. This is not to be part of the official distribution and it's the very first step - I am not sure how the next even looks like. ALso having the already finished parts available enabled other people to take a look.

Even though this isn't portable daca isn't either. And I think the first to integrate this with would be daca. But there's several other things I want to finish up first before taking a stab at that.

If the float.c is not executed.. only preprocessed.

That's a good point. So no compilation/execution is needed. I will adjust that in an upcoming PR.

@danmar
Copy link
Owner

danmar commented Oct 19, 2023

There's lots of unused and possibly broken stuff in the repo nobody has touched in years.

yes I realize that. Probably most of it was added by me myself.

And I think the first to integrate this with would be daca

then I understand why you wrote bash scripts at least.

@firewave
Copy link
Collaborator Author

There's lots of unused and possibly broken stuff in the repo nobody has touched in years.

yes I realize that. Probably most of it was added by me myself.

I have no idea why I even mentioned that - it's a really bad argument - heck, it's not an argument at all. 😉

@firewave firewave force-pushed the platform-limits branch 2 times, most recently from 9271570 to c5fa742 Compare October 19, 2023 19:14
@danmar
Copy link
Owner

danmar commented Oct 21, 2023

ok I hope this is work in progress and proper cleanup fixing will be added later..

@danmar danmar merged commit 3103736 into danmar:main Oct 21, 2023
74 checks passed
@firewave firewave deleted the platform-limits branch October 21, 2023 15:41
@firewave
Copy link
Collaborator Author

ok I hope this is work in progress and proper cleanup fixing will be added later..

You and me both. I am getting so sick that the local branches keep piling up...

@firewave
Copy link
Collaborator Author

This also (partially) fixes https://trac.cppcheck.net/ticket/10155.

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