Check for <= 0 sorta fails because of unsigned int / int mismatch

[mulle-nat]

Judging from the anteceding format (r=%d, c=%d)" nopts->row/col probably used to be int , but they are unsigned int actually:

notcurses.c:

  }else if(nopts->rows <= 0 || nopts->cols <= 0){
    logerror("won't create denormalized plane (r=%d, c=%d)",
             nopts->rows, nopts->cols);
    return NULL;

better:

  }else if( (int) nopts->rows <= 0 || (int) nopts->cols <= 0){
    logerror("won't create denormalized plane (r=%d, c=%d)",
             (int) nopts->rows, (int) nopts->cols);
    return NULL;

I fell into this, because I assumed I could use -1,-1 for the parent dimensions. Instead that netted me a very hard to find error in mbrtowc :roll_eyes:

[dankamongmen]

there's definitely a lot of sloppiness with the printf specifiers there =\

so that check ought not exist. i don't see why we'd cast it to int, though -- that's going to make it impossible to use the MSB, no? though maybe such a request is almost always invalid, and thus it's useful for us to catch it? but that violates the law of least astonishment imho.

fixing the specs, removing the check. some alloc failure will likely knock out a truly unworkable spec. thoughts?

[dankamongmen]

reopen if you think we need do something else.

[mulle-nat]

That may also be a newbie user problem. If I peruse the API, it's mostly fairly stringent unsigned usage for columns and rows. The function though, that I've been using the most so far API int ncplane_at_yx_cell(struct ncplane* n, int y, int x, nccell* c) does use int, so I was under the impression, you can't really address the MSB anyway. And the if(nopts->rows <= 0 || nopts->cols <= 0) snippet deepened that impression :wink:...

Anyway, what could then be nice, is to make a check against integer overflow, before the second malloc in ncplane_new_internal. This would catch the error also:

  size_t tmp, fbsize = sizeof(*p->fb);
  tmp = fbsize * p->leny;
  assert( tmp >= fbsize);
  fbsize = tmp;
  tmp = fbsize * p->lenx;
  assert( tmp >= fbsize);
  fbsize = tmp;

arguably not the most beautiful code.

[dankamongmen]

for those playing along at home, #2764 by @mulle-nat implements the plan he outlined above