Skip to content

drivers/pcd8544/pcd8544.c: fix var lengths#7859

Closed
kYc0o wants to merge 1 commit intoRIOT-OS:masterfrom
kYc0o:fix_pcd8544_var_len
Closed

drivers/pcd8544/pcd8544.c: fix var lengths#7859
kYc0o wants to merge 1 commit intoRIOT-OS:masterfrom
kYc0o:fix_pcd8544_var_len

Conversation

@kYc0o
Copy link
Copy Markdown
Contributor

@kYc0o kYc0o commented Oct 26, 2017

While compiling tests/driver_pcd8544 with clang in OS X it discovers several minor issues.

This PR fixes them.

@kYc0o kYc0o added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: drivers Area: Device drivers Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 26, 2017
aabadie
aabadie requested changes Oct 26, 2017
View reviewed changes
drivers/pcd8544/pcd8544.c
#define ASCII_MIN 0x20 /**< start of ASCII table */
#define ASCII_MAX 0x7e /**< end of ASCII table */
#define CHAR_WIDTH (6U) /**< pixel width of a single character */
#define CHAR_WIDTH (6) /**< pixel width of a single character */
Copy link
Copy Markdown
Contributor

@aabadie aabadie Oct 26, 2017

Choose a reason for hiding this comment

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

I don't think this change is needed, the previous version is more valid to me.

Copy link
Copy Markdown
Contributor Author

@kYc0o kYc0o Oct 26, 2017

Choose a reason for hiding this comment

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

Well the (6U) triggers several comparison warnings of different types. IMHO the U should be used when numbers are really high (maybe 16bit wide) to avoid casting errors on 8 bit platforms.

Thus in this case I don't see why it needs to be (6U).

Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen Oct 26, 2017

Choose a reason for hiding this comment

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

I am with @aabadie here: I'd prefer leaving the U. IMHO this should not only used with high numbers, but it should be used to mark any unsigned value... So the preferable fix here would be to actually fix the code that uses this define (there are only 2 lines effected...)

Copy link
Copy Markdown
Contributor Author

@kYc0o kYc0o Oct 26, 2017

Choose a reason for hiding this comment

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

I agree, just a remark: can a width be negative?

Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen Nov 9, 2017

Choose a reason for hiding this comment

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

nope

Copy link
Copy Markdown
Contributor Author

@kYc0o kYc0o Nov 21, 2017

Choose a reason for hiding this comment

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

Why to make it unsigned then?

Copy link
Copy Markdown
Contributor

@cladmi cladmi Nov 28, 2017

Choose a reason for hiding this comment

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

If there is a problem with the sign when using, it should be modified when using it not here.

Copy link
Copy Markdown
Contributor Author

@kYc0o kYc0o Nov 28, 2017

Choose a reason for hiding this comment

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

Ok will change...

@kYc0o kYc0o mentioned this pull request Oct 26, 2017
Closed
16 tasks
@kYc0o
Copy link
Copy Markdown
Contributor Author

kYc0o commented Nov 9, 2017

Ping!

@kYc0o
Copy link
Copy Markdown
Contributor Author

kYc0o commented Nov 10, 2017

Re-Ping!

@smlng
Copy link
Copy Markdown
Member

smlng commented Nov 13, 2017

@kYc0o you didn't address the comments, so do not ping until you did so. @haukepetersen and @aabadie (and I agree with them) that it should remain (6U) for CHAR_WIDTH, so adapt the code where it is used to fix any compiler warning, likely sign-compar�e.

@kYc0o
Copy link
Copy Markdown
Contributor Author

kYc0o commented Nov 17, 2017

I didn't receive the notifications for this one, sorry. But I don't see why leaving the U is correct. This is not the only case where it happens, I already fixed a some of them in my other PRs where the comparisons don't match, either by modifying the comparison in itself or by changing variable types. I don't think it's a good practice to systematically "unsign" the constants, which lead to this errors until #7919 is merged.

I really think that we need to address case by case when an unsigned constant is necessary and when it's not.

@haukepetersen
Copy link
Copy Markdown
Contributor

haukepetersen commented Nov 20, 2017

I really think that we need to address case by case when an unsigned constant is necessary and when it's not.

And in this case it is :-)

@smlng smlng added the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Nov 28, 2017
cladmi
cladmi suggested changes Nov 28, 2017
View reviewed changes
drivers/pcd8544/pcd8544.c
#define ASCII_MIN 0x20 /**< start of ASCII table */
#define ASCII_MAX 0x7e /**< end of ASCII table */
#define CHAR_WIDTH (6U) /**< pixel width of a single character */
#define CHAR_WIDTH (6) /**< pixel width of a single character */
Copy link
Copy Markdown
Contributor

@cladmi cladmi Nov 28, 2017

Choose a reason for hiding this comment

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

If there is a problem with the sign when using, it should be modified when using it not here.

@kYc0o
Copy link
Copy Markdown
Contributor Author

kYc0o commented Nov 28, 2017

Well anyways this was fixed by #7994 .

@kYc0o kYc0o closed this Nov 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@haukepetersen haukepetersen haukepetersen left review comments

@aabadie aabadie aabadie requested changes

+1 more reviewer

@cladmi cladmi cladmi requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@haukepetersen haukepetersen

Labels

Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@kYc0o @smlng @haukepetersen @cladmi @aabadie