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

Replaced PRId64 with PRIdPTR for intptr_t snprintf #3530

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@felixjones
Contributor

felixjones commented Mar 21, 2017

3703aed introduced the warning #3492 (comment)

mruby/mrbgems/mruby-compiler/core/codegen.c:2202:47:
warning: format specifies type 'long long' but the argument has type 'intptr_t' (aka 'long') [-Wformat]
snprintf(buf, sizeof(buf), "$%" PRId64, (intptr_t)tree);

PRIdPTR fixes this (expands to the correct format for intptr_t)

@matz

This comment has been minimized.

Show comment
Hide comment
@matz

matz Mar 21, 2017

Member

I concern that PRIdPTR available on many (or all) platforms.
I should have cast to int not intptr_t, maybe, since the value must be 1 digit integer.

Member

matz commented Mar 21, 2017

I concern that PRIdPTR available on many (or all) platforms.
I should have cast to int not intptr_t, maybe, since the value must be 1 digit integer.

@felixjones

This comment has been minimized.

Show comment
Hide comment
@felixjones

felixjones Mar 21, 2017

Contributor

I thought PRIdPTR was a C99 definition as part of inttypes along with PRId64?

Casting to int would be more ideal if 1 digit will only ever be needed, %d can be straight used in this case.

Contributor

felixjones commented Mar 21, 2017

I thought PRIdPTR was a C99 definition as part of inttypes along with PRId64?

Casting to int would be more ideal if 1 digit will only ever be needed, %d can be straight used in this case.

Felix Jones added some commits Mar 21, 2017

@felixjones

This comment has been minimized.

Show comment
Hide comment
@felixjones

felixjones Mar 21, 2017

Contributor

If 1 digit is expected, then can we use %1d and resize char buf[32] to char buf[3] ?

EDIT: After testing, %1d does not specify the maximum digits output, only the minimum (is in effect equivalent to %d).

Reducing buf to 3 works fine, with the most significant number being used in the buffer, for single digit integers this will always be the units, I think this would be acceptable as long as the case tree < 10 && tree >= 0 is true.

EDIT2: I'm also wondering if snprintf is even needed if tree is expected to be a 1 digit integer.

Is this not equivalent?

case NODE_NTH_REF:
    tree = (node*)((intptr_t)tree + '0'); // Mutates tree to be within ASCII range 0..9
    /* fall through */

case NODE_BACK_REF:
    if (val) {
      char buf[3];
      int sym;

      buf[0] = '$';
      buf[1] = (char)(intptr_t)tree;
      buf[2] = 0;
      sym = new_sym(s, mrb_intern_cstr(s->mrb, buf));
      genop(s, MKOP_ABx(OP_GETGLOBAL, cursp(), sym));
      push();
    }
    break;

If snprintf is to be used, I feel it should be used for safety and type check reasons, in which case I'd argue that PRIdPTR with an intptr_t cast should be used to allow for cases where tree < 0 || tree > 9

Contributor

felixjones commented Mar 21, 2017

If 1 digit is expected, then can we use %1d and resize char buf[32] to char buf[3] ?

EDIT: After testing, %1d does not specify the maximum digits output, only the minimum (is in effect equivalent to %d).

Reducing buf to 3 works fine, with the most significant number being used in the buffer, for single digit integers this will always be the units, I think this would be acceptable as long as the case tree < 10 && tree >= 0 is true.

EDIT2: I'm also wondering if snprintf is even needed if tree is expected to be a 1 digit integer.

Is this not equivalent?

case NODE_NTH_REF:
    tree = (node*)((intptr_t)tree + '0'); // Mutates tree to be within ASCII range 0..9
    /* fall through */

case NODE_BACK_REF:
    if (val) {
      char buf[3];
      int sym;

      buf[0] = '$';
      buf[1] = (char)(intptr_t)tree;
      buf[2] = 0;
      sym = new_sym(s, mrb_intern_cstr(s->mrb, buf));
      genop(s, MKOP_ABx(OP_GETGLOBAL, cursp(), sym));
      push();
    }
    break;

If snprintf is to be used, I feel it should be used for safety and type check reasons, in which case I'd argue that PRIdPTR with an intptr_t cast should be used to allow for cases where tree < 0 || tree > 9

@matz matz closed this in 9eb0c11 Mar 22, 2017

@matz

This comment has been minimized.

Show comment
Hide comment
@matz

matz Mar 22, 2017

Member

It can be more than 1 digit. $123456 is a valid variable name. Embarrassing.
I silenced the warning by 9eb0c11.

Member

matz commented Mar 22, 2017

It can be more than 1 digit. $123456 is a valid variable name. Embarrassing.
I silenced the warning by 9eb0c11.

matz added a commit that referenced this pull request Mar 22, 2017

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