Skip to content

Commit

Permalink
Use til::some<T,N> to replace the SomeViewports class (#4174)
Browse files Browse the repository at this point in the history
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request

Let's give it a test drive.

<!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> 
## References

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes #4162 
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) 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

<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed

Build and run it.
  • Loading branch information
skyline75489 authored and msftbot[bot] committed Jan 20, 2020
1 parent 62765f1 commit 69f3070
Show file tree
Hide file tree
Showing 5 changed files with 37 additions and 44 deletions.
14 changes: 1 addition & 13 deletions src/inc/til.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,23 +3,11 @@

#pragma once

#include "til/at.h"
#include "til/some.h"

namespace til // Terminal Implementation Library. Also: "Today I Learned"
{
// The at function declares that you've already sufficiently checked that your array access
// is in range before retrieving an item inside it at an offset.
// This is to save double/triple/quadruple testing in circumstances where you are already
// pivoting on the length of a set and now want to pull elements out of it by offset
// without checking again.
// gsl::at will do the check again. As will .at(). And using [] will have a warning in audit.
template<class T>
constexpr auto at(T& cont, const size_t i) -> decltype(cont[cont.size()])
{
#pragma warning(suppress : 26482) // Suppress bounds.2 check for indexing with constant expressions
#pragma warning(suppress : 26446) // Suppress bounds.4 check for subscript operator.
return cont[i];
}
}

// These sit outside the namespace because they sit outside for WIL too.
Expand Down
21 changes: 21 additions & 0 deletions src/inc/til/at.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT license.

#pragma once

namespace til
{
// The at function declares that you've already sufficiently checked that your array access
// is in range before retrieving an item inside it at an offset.
// This is to save double/triple/quadruple testing in circumstances where you are already
// pivoting on the length of a set and now want to pull elements out of it by offset
// without checking again.
// gsl::at will do the check again. As will .at(). And using [] will have a warning in audit.
template<class T>
constexpr auto at(T& cont, const size_t i) -> decltype(cont[cont.size()])
{
#pragma warning(suppress : 26482) // Suppress bounds.2 check for indexing with constant expressions
#pragma warning(suppress : 26446) // Suppress bounds.4 check for subscript operator.
return cont[i];
}
}
4 changes: 2 additions & 2 deletions src/inc/til/some.h
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned"
_outOfRange();
}

_array[_used] = val;
til::at(_array, _used) = val;

++_used;
}
Expand All @@ -168,7 +168,7 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned"

--_used;

_array[_used] = 0;
til::at(_array, _used) = 0;
}

[[noreturn]] void _invalidArg() const
Expand Down
28 changes: 5 additions & 23 deletions src/types/inc/viewport.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,16 @@ Author(s):

namespace Microsoft::Console::Types
{
struct SomeViewports;
class Viewport;

using SomeViewports = til::some<Viewport, 4>;

class Viewport final
{
public:
~Viewport() {}
constexpr Viewport() noexcept :
_sr({ 0, 0, -1, -1 }){};
Viewport(const Viewport& other) noexcept;
Viewport(Viewport&&) = default;
Viewport& operator=(const Viewport&) & = default;
Expand Down Expand Up @@ -125,28 +129,6 @@ namespace Microsoft::Console::Types
friend class ViewportTests;
#endif
};

struct SomeViewports final
{
unsigned char used{ 0 };
std::array<Viewport, 4> viewports{ Viewport::Empty(), Viewport::Empty(), Viewport::Empty(), Viewport::Empty() };

// These two methods are to make this vaguely look like a std::vector.

// Size is the number of viewports that are valid inside this structure
size_t size() const noexcept { return used; }

// At retrieves a viewport at a particular index. If you retrieve beyond the valid size(),
// it will throw std::out_of_range
const Viewport& at(size_t index) const
{
if (index >= used)
{
throw std::out_of_range("Access attempted beyond valid size.");
}
return viewports.at(index);
}
};
}

inline COORD operator-(const COORD& a, const COORD& b) noexcept
Expand Down
14 changes: 8 additions & 6 deletions src/types/viewport.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ Viewport::Viewport(const Viewport& other) noexcept :

Viewport Viewport::Empty() noexcept
{
return Viewport({ 0, 0, -1, -1 });
return Viewport();
}

Viewport Viewport::FromInclusive(const SMALL_RECT sr) noexcept
Expand Down Expand Up @@ -895,6 +895,7 @@ Viewport Viewport::ToOrigin() const noexcept
// that was covered by `main` before the regional area of `removeMe` was taken out.
// - You must check that each viewport .IsValid() before using it.
[[nodiscard]] SomeViewports Viewport::Subtract(const Viewport& original, const Viewport& removeMe) noexcept
try
{
SomeViewports result;

Expand All @@ -907,7 +908,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);
}
// If the original rectangle matches the intersection, there is nothing to return.
else if (original != intersection)
Expand Down Expand Up @@ -1003,27 +1004,28 @@ Viewport Viewport::ToOrigin() const noexcept

if (top.IsValid())
{
result.viewports.at(result.used++) = top;
result.push_back(top);
}

if (bottom.IsValid())
{
result.viewports.at(result.used++) = bottom;
result.push_back(bottom);
}

if (left.IsValid())
{
result.viewports.at(result.used++) = left;
result.push_back(left);
}

if (right.IsValid())
{
result.viewports.at(result.used++) = right;
result.push_back(right);
}
}

return result;
}
CATCH_FAIL_FAST()

// Method Description:
// - Returns true if the rectangle described by this Viewport has internal space
Expand Down

0 comments on commit 69f3070

Please sign in to comment.