Use til::some<T,N> to replace the SomeViewports class #4174

skyline75489 · 2020-01-10T01:40:07Z

Summary of the Pull Request

Let's give it a test drive.

References

PR Checklist

Closes Apply til::some<T,N> to replace the SomeViewports class #4162
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Requires documentation to be updated
I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Build and run it.

skyline75489 · 2020-01-10T01:41:17Z

src/types/viewport.cpp

@@ -6,6 +6,11 @@

 using namespace Microsoft::Console::Types;

+Viewport::Viewport() noexcept :


I didn't intend to add this. But the compiler complains about there is no default ctor when initializing til::some<Viewport, 4>result.

skyline75489 · 2020-01-10T02:00:20Z

Oops forget some files. Will look into it later.

DHowett-MSFT · 2020-01-10T02:34:51Z

src/types/inc/viewport.hpp

-    struct SomeViewports;
+    class Viewport;
+
+    typedef til::some<Viewport, 4> SomeViewports;


nit: prefer using for new code

DHowett-MSFT · 2020-01-10T02:35:07Z

src/types/inc/viewport.hpp


    class Viewport final
    {
    public:
        ~Viewport() {}
+        Viewport() noexcept;


interesting! I'm surprised this became necessary; perhaps it's something we could improve in some

I've tried bypass it by using SomeViewports result{Viewport::Empty x 4} but that would change the meaning of some, which I assume _used represents the number of element that's actually used. So I'm stucked here.

Unless Dustin has an idea of how to avoid this, I'm OK with the default constructor making an empty one. We're going to hopefully improve Viewport into a til::rect or something in the future anyway and a default constructor will almost certainly be warranted at that point in time.

Oh, I see. No, yeah, having a default constructor that does the same thing as Viewport::Empty is 100% the correct thing to do- to the point where I'm wondering why we ended up doing it not-that-way :P

We should do two or three things here.

make the constructor constexpr

make it initialize to the same values as Empty() (0, 0, -1, -1)

make it live in the header

4? make Empty just use the default ctor?

I will take a look at it. But I don’t really get it why a default is needed for the array. Perhaps I just don’t get C++...

Ah! Okay, so, std::array<T, 4> requires that storage be allocated for four things. Since array doesn't have a use counter, there must be a valid value of type T in every addressable index. Without a default constructor, the compiler cannot emit anything that would truly and safely initialize the values.

This is largely the same as Viewports v[4] -- each of the four viewports will be created and default-constructed.

Vector doesn't need to make this guarantee because accessing the objects beyond the end of the "used" region is undefined behavior.

Thanks Dustin. My admiration for you remains the same. My love for Java & C# just got a lot deeper.

zadjii-msft · 2020-01-10T13:31:12Z

@msftbot make sure @miniksa signs off on this

ghost · 2020-01-10T13:31:16Z

Hello @zadjii-msft!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by @miniksa

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

miniksa

I'm OK with this. I'd like the Dustin comments driven to conclusion before merge though.

skyline75489 · 2020-01-11T00:11:54Z

src/types/viewport.cpp

@@ -907,7 +912,7 @@ Viewport Viewport::ToOrigin() const noexcept
    if (!intersection.IsValid())
    {
        // Just put the original rectangle into the results and return early.
-        result.viewports.at(result.used++) = original;
+        result.push_back(original);


The compiler also complains about this throwing exception. But logically this method does not throw anything. And I'm surprised that the Substract is not used anywhere. Should I make it non-noexcept or just catch the exception inside it?

Oh, so this is an interesting one. push_back on some can throw an exception if it's full, so technically this one can throw an exception.
We know through static analysis (like, with our eyes) that it will never throw...

@miniksa what do you think about cases like this? This will never except until 4 becomes less than 3...

Nnnnn. A few ideas all of which I don't really like....

Make it except: That then pollutes all callers. Yuck.

Catch it: Seems obnoxious to write a whole try/CATCH_LOG or try/CATCH_FAIL_FAST around it.

Suppress it: A noexcept that throws technically goes straight to std::terminate which is more or less the same as try/CATCH_FAIL_FAST.

Add another accessor onto til::some. I can't think of one that wouldn't also suffer from this. If you do it with just unchecked array access, now you have a different warning and need til::at. If you do it with some.at(), that would presumably throw out of bounds too. If we add any other method, then it doesn't look quite like the STL collections it is mocking.

I'm leaning toward 2. I hate 1 and 4. I really don't like 3 as it's implicit instead of explicit like 2.
Doing 2 is also generally what we do around the codebase for this problem.

Also, this applies to .ToOrigin() below as well, right?

DHowett-MSFT · 2020-01-11T02:31:55Z

src/types/inc/viewport.hpp

@@ -13,15 +13,20 @@ Author(s):

 #pragma once
 #include "../../inc/operators.hpp"
+#include "../../inc/til.h"


if i recall, this one is included by default now!

It should be. This shouldn't be needed.

DHowett-MSFT · 2020-01-11T02:33:23Z

src/types/viewport.cpp

@@ -907,7 +912,7 @@ Viewport Viewport::ToOrigin() const noexcept
    if (!intersection.IsValid())
    {
        // Just put the original rectangle into the results and return early.
-        result.viewports.at(result.used++) = original;
+        result.push_back(original);


Oh, so this is an interesting one. push_back on some can throw an exception if it's full, so technically this one can throw an exception.
We know through static analysis (like, with our eyes) that it will never throw...

@miniksa what do you think about cases like this? This will never except until 4 becomes less than 3...

zadjii-msft · 2020-01-15T16:27:14Z

(this comment represents me recognizing that there's 2 green checks on this PR already, so I'm not going to review it personally. I'll let @miniksa and @DHowett-MSFT help resolve the static analysis build issues to merge this)

miniksa · 2020-01-16T22:15:55Z

OK, this one is now waiting for @skyline75489 to come back past one more time and probably try/catch guard the thing so it can pass static analysis. @DHowett-MSFT, sound good enough (out of the options I listed?)

skyline75489 · 2020-01-17T00:08:43Z

Updated the PR. @DHowett-MSFT @miniksa

miniksa · 2020-01-17T00:35:32Z

Nnnnn the static analysis check.

Can you create src/inc/til/at.h and move the definition of til::at in there, then do #include "til/at.h" before #include "til/some.h" in src/inc/til.h. Then finally, go put a til::at(_array, _used) = val on line 157 of src/inc/til/some.h?

EDIT: And probably also til::at(_array, _used) = 0 on line 171 as well?
EDIT 2: And wherever else the static analysis complains?

Sorry, this is new stuff and I hadn't worked out the kinks yet. I'd edit the branch for you, but I don't seem to have permission.

skyline75489 · 2020-01-17T14:32:45Z

Yeah it helps. The static analysis is good now.

zadjii-msft · 2020-01-20T20:29:41Z

/azp run

azure-pipelines · 2020-01-20T20:29:52Z

Azure Pipelines successfully started running 1 pipeline(s).

ghost · 2020-01-20T20:30:09Z

Hello @zadjii-msft!

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.

DHowett-MSFT · 2020-04-08T19:59:33Z

🎉 Once again, thanks for the contribution!

This pull request was included in a set of conhost changes that was just
released with Windows Insider Build 19603.

skyline75489 commented Jan 10, 2020

View reviewed changes

DHowett-MSFT reviewed Jan 10, 2020

View reviewed changes

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 10, 2020

zadjii-msft added the Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. label Jan 10, 2020

miniksa approved these changes Jan 10, 2020

View reviewed changes

miniksa removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 10, 2020

skyline75489 commented Jan 11, 2020

View reviewed changes

skyline75489 force-pushed the fix/today-i-learned-lesson-1 branch from 0d8f35d to 7842f3a Compare January 11, 2020 00:27

DHowett-MSFT reviewed Jan 11, 2020

View reviewed changes

DHowett-MSFT approved these changes Jan 11, 2020

View reviewed changes

skyline75489 force-pushed the fix/today-i-learned-lesson-1 branch from 7842f3a to 187422d Compare January 17, 2020 00:07

Use til::some<T,N> to replace the SomeViewports class

f8480b8

skyline75489 force-pushed the fix/today-i-learned-lesson-1 branch from 187422d to f8480b8 Compare January 17, 2020 14:29

Merge branch 'master' into fix/today-i-learned-lesson-1

969fd69

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 20, 2020

ghost merged commit 69f3070 into microsoft:master Jan 20, 2020

skyline75489 deleted the fix/today-i-learned-lesson-1 branch January 25, 2021 02:58

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use til::some<T,N> to replace the SomeViewports class #4174

Use til::some<T,N> to replace the SomeViewports class #4174

skyline75489 commented Jan 10, 2020 •

edited

Loading

skyline75489 Jan 10, 2020

skyline75489 commented Jan 10, 2020

DHowett-MSFT Jan 10, 2020

DHowett-MSFT Jan 10, 2020

skyline75489 Jan 10, 2020

miniksa Jan 10, 2020

DHowett-MSFT Jan 10, 2020

DHowett-MSFT Jan 10, 2020 •

edited

Loading

skyline75489 Jan 10, 2020

DHowett-MSFT Jan 10, 2020 •

edited

Loading

DHowett-MSFT Jan 10, 2020

skyline75489 Jan 11, 2020

zadjii-msft commented Jan 10, 2020

ghost commented Jan 10, 2020

miniksa left a comment

skyline75489 Jan 11, 2020

DHowett-MSFT Jan 11, 2020

miniksa Jan 16, 2020

DHowett-MSFT Jan 11, 2020

miniksa Jan 16, 2020

DHowett-MSFT Jan 11, 2020

zadjii-msft commented Jan 15, 2020

miniksa commented Jan 16, 2020

skyline75489 commented Jan 17, 2020

miniksa commented Jan 17, 2020 •

edited

Loading

skyline75489 commented Jan 17, 2020

zadjii-msft commented Jan 20, 2020

azure-pipelines bot commented Jan 20, 2020

ghost commented Jan 20, 2020

DHowett-MSFT commented Apr 8, 2020

		@@ -6,6 +6,11 @@

		using namespace Microsoft::Console::Types;

		Viewport::Viewport() noexcept :

Use til::some<T,N> to replace the SomeViewports class #4174

Use til::some<T,N> to replace the SomeViewports class #4174

Conversation

skyline75489 commented Jan 10, 2020 • edited Loading

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Choose a reason for hiding this comment

skyline75489 commented Jan 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett-MSFT Jan 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett-MSFT Jan 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zadjii-msft commented Jan 10, 2020

ghost commented Jan 10, 2020

miniksa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zadjii-msft commented Jan 15, 2020

miniksa commented Jan 16, 2020

skyline75489 commented Jan 17, 2020

miniksa commented Jan 17, 2020 • edited Loading

skyline75489 commented Jan 17, 2020

zadjii-msft commented Jan 20, 2020

azure-pipelines bot commented Jan 20, 2020

ghost commented Jan 20, 2020

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.

DHowett-MSFT commented Apr 8, 2020

skyline75489 commented Jan 10, 2020 •

edited

Loading

DHowett-MSFT Jan 10, 2020 •

edited

Loading

DHowett-MSFT Jan 10, 2020 •

edited

Loading

miniksa commented Jan 17, 2020 •

edited

Loading

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.