Skip to content

Commit

Permalink
Use the til::enumset type for the SgrSaveRestoreStackOptions enum (#1…
Browse files Browse the repository at this point in the history 
…1888)

## Summary of the Pull Request

This replaces the `std::bitset` for the `SgrSaveRestoreStackOptions` enum with a `til::enumset` type, thus avoiding the need to cast the `enum` to a `size_t` every time a value is set or tested.

This also fixes an issue with the handling of omitted and zero parameters in the `XTPUSHSGR` sequence, which are meant to be ignored, and not interpreted as "all".

## PR Checklist
* [x] Closes #11879
* [x] Closes #11883
* [x] CLA signed.
* [ ] Tests added/passed
* [ ] Documentation updated.
* [ ] Schema 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

In addition to dropping all the `static_cast` operations, the use of the `til::enumset` also allowed us to get rid of the `try`/`catch` handling that was previously required in a couple of places, since the `til::enumset` operations don't throw. 

And to fix the zero parameter handling, we just needed to add an additional lower bound when validating that options are in range - if an option is 0 (`All`), it will now just be ignored.

## Validation Steps Performed

The updated code still passes the existing unit tests, and I've manually confirmed that it fixes the test case for omitted and zero parameters from issue #11883.
  • Loading branch information
@j4james
j4james authored Dec 7, 2021
1 parent ca20bbd commit 91ad17d
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 67 deletions.
13 changes: 5 additions & 8 deletions src/types/inc/sgrStack.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@ Module Name:

#pragma once

#include "..\..\buffer\out\TextAttribute.hpp"
#include "..\..\terminal\adapter\DispatchTypes.hpp"
#include <bitset>
#include "../../buffer/out/TextAttribute.hpp"
#include "../../terminal/adapter/DispatchTypes.hpp"
#include <til/enumset.h>

namespace Microsoft::Console::VirtualTerminal
{
Expand Down Expand Up @@ -51,14 +51,11 @@ namespace Microsoft::Console::VirtualTerminal
static constexpr int c_MaxStoredSgrPushes = 10;

private:
// Note the +1 in the size of the bitset: this is because we use the
// SgrSaveRestoreStackOptions enum values as bitset flags, so they are naturally
// one-based.
typedef std::bitset<static_cast<size_t>(DispatchTypes::SgrSaveRestoreStackOptions::Max) + 1> AttrBitset;
typedef til::enumset<DispatchTypes::SgrSaveRestoreStackOptions> AttrBitset;

TextAttribute _CombineWithCurrentAttributes(const TextAttribute& currentAttributes,
const TextAttribute& savedAttribute,
const AttrBitset validParts); // valid parts of savedAttribute
const AttrBitset validParts) noexcept; // valid parts of savedAttribute

struct SavedSgrAttributes
{
Expand Down
96 changes: 37 additions & 59 deletions src/types/sgrStack.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,42 +19,31 @@ namespace Microsoft::Console::VirtualTerminal
{
AttrBitset validParts;

try
if (options.empty())
{
if (options.empty())
{
// We save all current attributes.
validParts.set(static_cast<size_t>(SgrSaveRestoreStackOptions::All));
}
else
// We save all current attributes.
validParts.set(SgrSaveRestoreStackOptions::All);
}
else
{
// Each option is encoded as a bit in validParts. All options (that fit) are
// encoded; options that aren't supported are ignored when read back (popped).
// So if you try to save only unsupported aspects of the current text
// attributes, you'll do what is effectively an "empty" push (the subsequent
// pop will not change the current attributes), which is the correct behavior.

for (size_t i = 0; i < options.size(); i++)
{
// Each option is encoded as a bit in validParts. All options (that fit) are
// encoded; options that aren't supported are ignored when read back (popped).
// So if you try to save only unsupported aspects of the current text
// attributes, you'll do what is effectively an "empty" push (the subsequent
// pop will not change the current attributes), which is the correct behavior.
const auto option = static_cast<SgrSaveRestoreStackOptions>(options.at(i).value_or(0));

for (size_t i = 0; i < options.size(); i++)
// Options must be specified singly; not in combination. Values that are
// out of range will be ignored.
if (option > SgrSaveRestoreStackOptions::All && option <= SgrSaveRestoreStackOptions::Max)
{
const size_t optionAsIndex = options.at(i).value_or(0);

// Options must be specified singly; not in combination. Values that are
// out of range will be ignored.
if (optionAsIndex < validParts.size())
{
validParts.set(optionAsIndex);
}
validParts.set(option);
}
}
}
catch (...)
{
// The static analyzer knows that the bitset operations can throw
// std::out_of_range. However, we know that won't happen, because we pre-check
// that everything should be in range. So we plan to never execute this
// failfast:
FAIL_FAST_CAUGHT_EXCEPTION();
}

if (_numSavedAttrs < gsl::narrow<int>(_storedSgrAttributes.size()))
{
Expand Down Expand Up @@ -82,26 +71,15 @@ namespace Microsoft::Console::VirtualTerminal

SavedSgrAttributes& restoreMe = _storedSgrAttributes.at(_nextPushIndex);

try
if (restoreMe.ValidParts.test(SgrSaveRestoreStackOptions::All))
{
if (restoreMe.ValidParts.test(static_cast<size_t>(SgrSaveRestoreStackOptions::All)))
{
return restoreMe.TextAttributes;
}
else
{
return _CombineWithCurrentAttributes(currentAttributes,
restoreMe.TextAttributes,
restoreMe.ValidParts);
}
return restoreMe.TextAttributes;
}
catch (...)
else
{
// The static analyzer knows that the bitset operations can throw
// std::out_of_range. However, we know that won't happen, because we
// pre-check that everything should be in range. So we plan to never
// execute this failfast:
FAIL_FAST_CAUGHT_EXCEPTION();
return _CombineWithCurrentAttributes(currentAttributes,
restoreMe.TextAttributes,
restoreMe.ValidParts);
}
}

Expand All @@ -110,11 +88,11 @@ namespace Microsoft::Console::VirtualTerminal

TextAttribute SgrStack::_CombineWithCurrentAttributes(const TextAttribute& currentAttributes,
const TextAttribute& savedAttribute,
const AttrBitset validParts) // of savedAttribute
const AttrBitset validParts) noexcept // of savedAttribute
{
// If we are restoring all attributes, we should have just taken savedAttribute
// before we even got here.
FAIL_FAST_IF(validParts.test(static_cast<size_t>(SgrSaveRestoreStackOptions::All)));
FAIL_FAST_IF(validParts.test(SgrSaveRestoreStackOptions::All));

TextAttribute result = currentAttributes;

Expand All @@ -141,67 +119,67 @@ namespace Microsoft::Console::VirtualTerminal
// (some closing braces for people with editors that get thrown off without them: }})

// Boldness = 1,
if (validParts.test(static_cast<size_t>(SgrSaveRestoreStackOptions::Boldness)))
if (validParts.test(SgrSaveRestoreStackOptions::Boldness))
{
result.SetBold(savedAttribute.IsBold());
}

// Faintness = 2,
if (validParts.test(static_cast<size_t>(SgrSaveRestoreStackOptions::Faintness)))
if (validParts.test(SgrSaveRestoreStackOptions::Faintness))
{
result.SetFaint(savedAttribute.IsFaint());
}

// Italics = 3,
if (validParts.test(static_cast<size_t>(SgrSaveRestoreStackOptions::Italics)))
if (validParts.test(SgrSaveRestoreStackOptions::Italics))
{
result.SetItalic(savedAttribute.IsItalic());
}

// Underline = 4,
if (validParts.test(static_cast<size_t>(SgrSaveRestoreStackOptions::Underline)))
if (validParts.test(SgrSaveRestoreStackOptions::Underline))
{
result.SetUnderlined(savedAttribute.IsUnderlined());
}

// Blink = 5,
if (validParts.test(static_cast<size_t>(SgrSaveRestoreStackOptions::Blink)))
if (validParts.test(SgrSaveRestoreStackOptions::Blink))
{
result.SetBlinking(savedAttribute.IsBlinking());
}

// Negative = 7,
if (validParts.test(static_cast<size_t>(SgrSaveRestoreStackOptions::Negative)))
if (validParts.test(SgrSaveRestoreStackOptions::Negative))
{
result.SetReverseVideo(savedAttribute.IsReverseVideo());
}

// Invisible = 8,
if (validParts.test(static_cast<size_t>(SgrSaveRestoreStackOptions::Invisible)))
if (validParts.test(SgrSaveRestoreStackOptions::Invisible))
{
result.SetInvisible(savedAttribute.IsInvisible());
}

// CrossedOut = 9,
if (validParts.test(static_cast<size_t>(SgrSaveRestoreStackOptions::CrossedOut)))
if (validParts.test(SgrSaveRestoreStackOptions::CrossedOut))
{
result.SetCrossedOut(savedAttribute.IsCrossedOut());
}

// DoublyUnderlined = 21,
if (validParts.test(static_cast<size_t>(SgrSaveRestoreStackOptions::DoublyUnderlined)))
if (validParts.test(SgrSaveRestoreStackOptions::DoublyUnderlined))
{
result.SetDoublyUnderlined(savedAttribute.IsDoublyUnderlined());
}

// SaveForegroundColor = 30,
if (validParts.test(static_cast<size_t>(SgrSaveRestoreStackOptions::SaveForegroundColor)))
if (validParts.test(SgrSaveRestoreStackOptions::SaveForegroundColor))
{
result.SetForeground(savedAttribute.GetForeground());
}

// SaveBackgroundColor = 31,
if (validParts.test(static_cast<size_t>(SgrSaveRestoreStackOptions::SaveBackgroundColor)))
if (validParts.test(SgrSaveRestoreStackOptions::SaveBackgroundColor))
{
result.SetBackground(savedAttribute.GetBackground());
}
Expand Down

0 comments on commit 91ad17d

Please sign in to comment.