Skip to content

cflags: add -Wformat=2 -Wformat-overflow -Wformat-truncation#9355

Merged
miri64 merged 5 commits intoRIOT-OS:masterfrom
jnohlgard:pr/Wformat
Jul 19, 2018
Merged

cflags: add -Wformat=2 -Wformat-overflow -Wformat-truncation#9355
miri64 merged 5 commits intoRIOT-OS:masterfrom
jnohlgard:pr/Wformat

Conversation

@jnohlgard
Copy link
Copy Markdown
Member

@jnohlgard jnohlgard commented Jun 14, 2018

Contribution description

Enables additional compile time checks for format strings used by printf, scanf, strftime.
Includes a fix for warnings in the (deprecated) sys/cbor module

Issues/PRs references

Related: #9243
Depends on: #9358, #9357

@jnohlgard jnohlgard added Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 14, 2018
@jnohlgard jnohlgard added this to the Release 2018.07 milestone Jun 14, 2018
@jnohlgard jnohlgard requested a review from cladmi June 14, 2018 20:54
@jnohlgard jnohlgard added the State: waiting for other PR State: The PR requires another PR to be merged first label Jun 14, 2018
@jnohlgard jnohlgard force-pushed the pr/Wformat branch 2 times, most recently from 0c1bebf to 404ff8d Compare June 26, 2018 20:03
struct tm timeinfo;
size_t read_bytes = cbor_deserialize_date_time(stream, offset, &timeinfo);
strftime(buf, sizeof(buf), "%c", &timeinfo);
strftime(buf, sizeof(buf), TIMESTRING_FORMAT, &timeinfo);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Here, "%c" has been replaced by a date format. Was that an old remaining bug ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

One of the warnings activated in this PR triggered on %c. I don't remember which warning, I just wanted to fix this in the least intrusive way possible since the module is deprecated and will be removed soon

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry, I missed that it was strftime and not just printf which is why I did not understand.
And yes it silents the warning on native.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just for reference, if someone is interested, this date output changed:

  • from: Thu Jul 5 15:57:27 2018
  • to: 2018-07-05T15:57:48Z

Copy link
Copy Markdown
Contributor

@cladmi cladmi left a comment

Choose a reason for hiding this comment

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

Diff looks good, murdock is happy, ACK.

Just waiting for the other PR and rebase after.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jul 18, 2018

Please rebase. #9358 was merged.

@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Jul 18, 2018

Done

@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR State: waiting for other PR State: The PR requires another PR to be merged first labels Jul 18, 2018
@cladmi cladmi added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jul 19, 2018
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

(re-ACK)

@miri64 miri64 merged commit 51a9ac1 into RIOT-OS:master Jul 19, 2018
@miri64 miri64 added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Jul 19, 2018
@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Jul 19, 2018

Backport provided in #9608

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants