Skip to content

Conversation

@carlos-zamora
Copy link
Member

Found a bug where the following won't work:

COORD inclusiveEnd{ _end };

where _end is a til::point.

The only fix for this is to replace these instances with this:

COORD inclusiveEnd = _end;

What was happening in the first notation is the implicit conversion of til::point to bool to SHORT. The constructor for COORD only sees one SHORT so it thinks the value should be the definition for X, and Y should stay as 0. So we end up getting 1, 0.

By adding the explicit keyword to the bool operators, we prevent the accident above from occurring.

@carlos-zamora carlos-zamora requested a review from miniksa March 16, 2020 21:23
@miniksa
Copy link
Member

miniksa commented Mar 16, 2020

Did you forget the one on size.h or did I not put one there?

@carlos-zamora
Copy link
Member Author

Did you forget the one on size.h or did I not put one there?

surprisingly, there isn't one there. Want me to add one in?

@miniksa
Copy link
Member

miniksa commented Mar 16, 2020

Did you forget the one on size.h or did I not put one there?

surprisingly, there isn't one there. Want me to add one in?

Eh, only if you need it.

@miniksa
Copy link
Member

miniksa commented Mar 16, 2020

Did you forget the one on size.h or did I not put one there?

surprisingly, there isn't one there. Want me to add one in?

Eh, only if you need it.

Or whatever, I see you already did. It's fine.

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

Wait could we add a test for the case you explicitly put in the description?

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 17, 2020
@carlos-zamora
Copy link
Member Author

Wait could we add a test for the case you explicitly put in the description?

Doing something like this:

        const til::point pt{ 1, 1 };
        COORD cd{ pt };

won't even compile. So, I think there isn't a way we can test it.

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 17, 2020
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

Oh well that'll do for me, thanks!

@zadjii-msft zadjii-msft added the Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. label Mar 19, 2020
@miniksa miniksa added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 19, 2020
@ghost
Copy link

ghost commented Mar 19, 2020

Hello @miniksa!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit ae3f8f3 into master Mar 19, 2020
@ghost ghost deleted the dev/cazamor/til-explicit-bool branch March 19, 2020 16:12
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. AutoMerge Marked for automatic merge by the bot when requirements are met

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants