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

Add format checking to printf-type APIs #8601

Merged
merged 8 commits into from
Nov 1, 2018

Conversation

kjbracey
Copy link
Contributor

@kjbracey kjbracey commented Oct 31, 2018

Description

Toolchains are capable of performing format-checking on user-defined printf variants. Add the necessary directives to do this for some commonly-used mbed OS APIs:

  • error
  • mbed_error_printf
  • debug, debug_if
  • Stream::printf, Stream::scanf, RawSerial::printf

Some clean-ups of warnings generated by this change. SDBlockDevice changes are most wide-ranging - it's using 64-bit arithmetic excessively due to bd_size_t being used for all sorts of things that must be much smaller (being limited to size_t by being in-memory, or just block sizes).

Pull request type

[ ] Fix
[X] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Correct types passed to debug() calls - as part of this, block size and
erase size are changed to be 32-bit - using 64-bit variables for these
will cause unnecessary code bloat.

Many uses of bd_size_t in the BlockDevice API should really be uint32_t
or size_t, but that would be a bigger API change.
@0xc0170 0xc0170 requested a review from a team October 31, 2018 13:41
@kjbracey kjbracey requested a review from a team October 31, 2018 13:46
Copy link

@deepikabhavnani deepikabhavnani left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍 Thanks!!

Copy link
Contributor

@geky geky left a comment

Choose a reason for hiding this comment

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

Nice 👍 Looks good to me. In hindsight we should have used size_t for the BlockDevice functions you outlined, but unfortunately it will be hard to change at this point.

@cmonr
Copy link
Contributor

cmonr commented Oct 31, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Nov 1, 2018

Build : SUCCESS

Build number : 3525
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/8601/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented Nov 1, 2018

@kjbracey
Copy link
Contributor Author

kjbracey commented Nov 1, 2018

I did try adjusting those BlockDevice things to size_t, in the thought that there were few enough implementations to make it viable, but soon became apparent how many nice composition classes you had that they passed through, and gave up.

Not sure it'd make a massive difference anyway - I fixed what seemed to be the most wasteful, dividing by a 64-bit variable block size when it's a constant 512, but even that only seemed to shave a few bytes off. (Although maybe a bit more trimming elsewhere would eventually allow a 64/64 divide routine to be omitted from the library).

@mbed-ci
Copy link

mbed-ci commented Nov 1, 2018

@cmonr
Copy link
Contributor

cmonr commented Nov 1, 2018

/morph test

@mbed-ci
Copy link

mbed-ci commented Nov 1, 2018

@cmonr cmonr merged commit 430fb3e into ARMmbed:master Nov 1, 2018
@cmonr cmonr removed the needs: CI label Nov 1, 2018
@kjbracey kjbracey deleted the error_fmtcheck branch November 2, 2018 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants