1610670 - Create iterable collections for all values for LogicalSide, mozilla::Side, etc to support range-for loop

Status: RESOLVED FIXED

(Core :: Layout, task, P3)

Description

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Filed mats' review comments in https://phabricator.services.mozilla.com/D59051?id=219067#inline-365769.

And we can replace NS_FOR_CSS_SIDES usages to modern range-for loops.

Comment 1

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

FWIW, we have MakeEnumeratedRange(EnumType aBegin, EnumType aEnd), but the helper class requires the enum to have an end guard (because aEnd is exclusive). Since LogicalSide, mozilla::Side, etc. are unlikely to add any new enum value. I feel maintaining constexpr array for all values in the enum is the way to go.

Out of curiosity, I wrote a short example to see the assembly code generated from the ranged-for loops. It turns out that clang is smart enough to expand the loop under -O flag. https://gcc.godbolt.org/z/_E7ehi

Comment 2

[Emilio Cobos Álvarez (:emilio)]

It could be better / also possible to add something like:

template <typename EnumType>
inline detail::EnumeratedRange<EnumType> MakeInclusiveEnumeratedRange(EnumType aBegin, EnumType aEnd) {
  MOZ_ASSERT(aEnd != std::numeric_limits<std::underlying_type<EnumType>>::max_value(), "Shouldn't overflow below");
  return MakeEnumeratedRange(aBegin, static_cast<EnumType>(static_cast<std::underlying_type<EnumType>(aEnd) + 1));
}

Or something like that?

Comment 3

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Re comment 2:

I was worry about it's an undefined behavior that static_cast from an integer value (i.e. aEnd+1) to an enum that is not defined in the range. But it seems OK per https://stackoverflow.com/questions/55392083/c-enum-class-cast-to-non-existing-entry. So I filed bug 1610980, and I will steal your idea and transform it into a working patch :)

Comment 4

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1610670 - Add AllLogicalSides() to support range-based for loops. r=mats

Comment 5

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1610670 - Add AllPhysicalSides() to support range-based for loops. r=mats

This patch is generated via:

1. Manually modify gfx/2d/Types.h
2. Run the following script and clang-format.

#!/bin/bash

function rename() {
    echo "Renaming $1 to $2"
    rg -l "$1" | xargs sed -i -E -e s/"$1"/"$2"/g
}

rename "NS_FOR_CSS_SIDES\(side\)" "for (const auto side : mozilla::AllSides())"
rename "NS_FOR_CSS_SIDES\(s\)" "for (const auto s : mozilla::AllSides())"
rename "NS_FOR_CSS_SIDES\(i\)" "for (const auto i : mozilla::AllSides())"
rename "NS_FOR_CSS_SIDES\(ix\)" "for (const auto ix : mozilla::AllSides())"

Depends on D60754

Comment 6

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1610670 - Add AllPhysicalCorners() to support range-based for loops. r=mats

This patch is generated via:

1. Manually modify gfx/2d/Types.h
2. Run the following script and clang-format.

#!/bin/bash

function rename() {
    echo "Renaming $1 to $2"
    rg -l "$1" | xargs sed -i -E -e s/"$1"/"$2"/g
}

rename "NS_FOR_CSS_FULL_CORNERS\(i\)" "for (const auto i : mozilla::AllCorners())"
rename "NS_FOR_CSS_FULL_CORNERS\(corner\)" "for (const auto corner : mozilla::AllCorners())"

Depends on D61250

Comment 7

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1610670 - Add AllPhysicalHalfCorners() to support range-based for loops. r=mats

This patch is generated via:

1. Manually modify gfx/2d/Types.h
2. Run the following script and clang-format.
3. Add brackets for the for loop in nsCSSRendering.cpp.

#!/bin/bash

function rename() {
    echo "Renaming $1 to $2"
    rg -l "$1" | xargs sed -i -E -e s/"$1"/"$2"/g
}

rename "NS_FOR_CSS_HALF_CORNERS\(i\)" "for (const auto i : mozilla::AllHalfCorners())"
rename "NS_FOR_CSS_HALF_CORNERS\(corner\)" "for (const auto corner : mozilla::AllHalfCorners())"

Depends on D61251

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e547c92c8e71
Add AllLogicalSides() to support range-based for loops. r=mats
https://hg.mozilla.org/integration/autoland/rev/ae9aa0634f7c
Add AllPhysicalSides() to support range-based for loops. r=mats
https://hg.mozilla.org/integration/autoland/rev/06b4c98d7731
Add AllPhysicalCorners() to support range-based for loops. r=mats
https://hg.mozilla.org/integration/autoland/rev/b95921676bb4
Add AllPhysicalHalfCorners() to support range-based for loops. r=mats

Comment 9

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/e547c92c8e71
https://hg.mozilla.org/mozilla-central/rev/ae9aa0634f7c
https://hg.mozilla.org/mozilla-central/rev/06b4c98d7731
https://hg.mozilla.org/mozilla-central/rev/b95921676bb4