Explicit narrow cast to short

[refack]

To get this to build with CL 19.16.27024.1 on SDK 10.0.18282.0
Eliminate Warning C4838: conversion from [X] to 'SHORT' requires a narrowing conversion

[henrik-jensen]

Was about to make a PR regarding the same error until I noticed your (@refack's) PR.
CL 19.16.27025.1 with SDK 8.1 ( default created by premake5 vs2017)
My PR was essentially the same though I choose to use static_cast. Wonder if there would be any difference?

//------------------------------------------------------------------------------
void win_screen_buffer::set_cursor(int column, int row)
{
    CONSOLE_SCREEN_BUFFER_INFO csbi;
    GetConsoleScreenBufferInfo(m_handle, &csbi);

    const SMALL_RECT& window = csbi.srWindow;
    int width = (window.Right - window.Left) + 1;
    int height = (window.Bottom - window.Top) + 1;

    column = clamp(column, 0, width);
    row = clamp(row, 0, height);

    COORD xy = { static_cast<SHORT> (window.Left + column), static_cast<SHORT> (window.Top + row) };
    SetConsoleCursorPosition(m_handle, xy);
}

//------------------------------------------------------------------------------
void win_screen_buffer::move_cursor(int dx, int dy)
{
    CONSOLE_SCREEN_BUFFER_INFO csbi;
    GetConsoleScreenBufferInfo(m_handle, &csbi);

    COORD xy = {
		static_cast<SHORT> (clamp(csbi.dwCursorPosition.X + dx, 0, csbi.dwSize.X - 1)),
		static_cast<SHORT> (clamp(csbi.dwCursorPosition.Y + dy, 0, csbi.dwSize.Y - 1)),
    };
    SetConsoleCursorPosition(m_handle, xy);
}

[refack]

Wonder if there would be any difference?

I just don't like "recycling" variables, as per ES.26: Don’t use a variable for two unrelated purposes

[henrik-jensen]

Wonder if there would be any difference?

I just don't like "recycling" variables, as per ES.26: Don’t use a variable for two unrelated purposes

Makes sense.
Though you do hide the implicit conversion, which is explicit with a static_cast, it is admittedly very easy to spot.
Anyway maybe the whole win_screen_buffer interface should be refactored to take SHORT's instead of int's, considering they all call windows api's that takes SHORTS or structs of SHORTS, but that would be another PR (and probably can of worms).

[refack]

Though you do hide the implicit conversion

That's true, but since they store the result of clamp I wasn't too worried ¯\(ツ)/¯

Anyway maybe the whole win_screen_buffer api should be refactored to take SHORT's instead of int's...

Sounds like a good idea. But anyway I'm not sure how actively maintained this repo is. My PR has been cooking for 20 days...

[henrik-jensen]

Well it's the holidays right now (Ok it's 5 month since the last change, maybe the project is a bit stalled). Btw,- Happy holidays (and merry christmas if you are of a christian denomination) :).

A bit OC: I'm not especially versed in github. I'm working on another PR and obviously it depends on your not yet merged PR to even build. To avoid later merge conflicts, I wonder how the right procedure is. Should I just add your changes to my local repo and then make the PR or does github have some mechanism to say that my PR is dependent on your PR?
The first one would steal your credits for the PR if merged first, which is not fair, so I would prefer another solution.

[henrik-jensen]

Found a solution. I just download your PR as a patch (you know the trick with appending the PR url with .patch) which retains your credits. Run it on my local repo, commits it and then make my own changes and request a PR. That should do it I think.

[Stanzilla]

I would not expect any response on tickets or pull requests, even though @mridgers pushes code sometimes, he does not really communicate anymore.