Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
8257912: Convert enum iteration to use range-based for loops
Reviewed-by: kbarrett, tschatzl, gziemski
  • Loading branch information
iklam committed Dec 10, 2020
1 parent 164c55b commit 80dac5a
Show file tree
Hide file tree
Showing 10 changed files with 95 additions and 48 deletions.
6 changes: 2 additions & 4 deletions src/hotspot/share/ci/ciObjectFactory.cpp
Expand Up @@ -122,17 +122,15 @@ void ciObjectFactory::init_shared_objects() {

{
// Create the shared symbols, but not in _shared_ci_metadata.
for (vmSymbolsIterator it = vmSymbolsRange.begin(); it != vmSymbolsRange.end(); ++it) {
vmSymbolID index = *it;
for (vmSymbolID index : EnumRange<vmSymbolID>{}) {
Symbol* vmsym = vmSymbols::symbol_at(index);
assert(vmSymbols::find_sid(vmsym) == index, "1-1 mapping");
ciSymbol* sym = new (_arena) ciSymbol(vmsym, index);
init_ident_of(sym);
_shared_ci_symbols[vmSymbols::as_int(index)] = sym;
}
#ifdef ASSERT
for (vmSymbolsIterator it = vmSymbolsRange.begin(); it != vmSymbolsRange.end(); ++it) {
vmSymbolID index = *it;
for (vmSymbolID index : EnumRange<vmSymbolID>{}) {
Symbol* vmsym = vmSymbols::symbol_at(index);
ciSymbol* sym = vm_symbol_at(index);
assert(sym->get_symbol() == vmsym, "oop must match");
Expand Down
3 changes: 1 addition & 2 deletions src/hotspot/share/classfile/classFileParser.cpp
Expand Up @@ -5302,8 +5302,7 @@ static void check_methods_for_intrinsics(const InstanceKlass* ik,
// The check is potentially expensive, therefore it is available
// only in debug builds.

for (vmIntrinsicsIterator it = vmIntrinsicsRange.begin(); it != vmIntrinsicsRange.end(); ++it) {
vmIntrinsicID id = *it;
for (vmIntrinsicID id : EnumRange<vmIntrinsicID>{}) {
if (vmIntrinsics::_compiledLambdaForm == id) {
// The _compiledLamdbdaForm intrinsic is a special marker for bytecode
// generated for the JVM from a LambdaForm and therefore no method
Expand Down
6 changes: 2 additions & 4 deletions src/hotspot/share/classfile/vmIntrinsics.cpp
Expand Up @@ -574,8 +574,7 @@ void vmIntrinsics::init_vm_intrinsic_name_table() {
const char** nt = &vm_intrinsic_name_table[0];
char* string = (char*) &vm_intrinsic_name_bodies[0];

for (vmIntrinsicsIterator it = vmIntrinsicsRange.begin(); it != vmIntrinsicsRange.end(); ++it) {
vmIntrinsicID index = *it;
for (vmIntrinsicID index : EnumRange<vmIntrinsicID>{}) {
nt[as_int(index)] = string;
string += strlen(string); // skip string body
string += 1; // skip trailing null
Expand All @@ -602,8 +601,7 @@ vmIntrinsics::ID vmIntrinsics::find_id(const char* name) {
init_vm_intrinsic_name_table();
}

for (vmIntrinsicsIterator it = vmIntrinsicsRange.begin(); it != vmIntrinsicsRange.end(); ++it) {
vmIntrinsicID index = *it;
for (vmIntrinsicID index : EnumRange<vmIntrinsicID>{}) {
if (0 == strcmp(name, nt[as_int(index)])) {
return index;
}
Expand Down
2 changes: 0 additions & 2 deletions src/hotspot/share/classfile/vmIntrinsics.hpp
Expand Up @@ -1044,8 +1044,6 @@ enum class vmIntrinsicID : int {
};

ENUMERATOR_RANGE(vmIntrinsicID, vmIntrinsicID::FIRST_ID, vmIntrinsicID::LAST_ID)
constexpr EnumRange<vmIntrinsicID> vmIntrinsicsRange; // the default range of all valid vmIntrinsicIDs
using vmIntrinsicsIterator = EnumIterator<vmIntrinsicID>; // convenience

class vmIntrinsics : AllStatic {
friend class vmSymbols;
Expand Down
30 changes: 11 additions & 19 deletions src/hotspot/share/classfile/vmSymbols.cpp
Expand Up @@ -83,8 +83,7 @@ void vmSymbols::initialize(TRAPS) {

if (!UseSharedSpaces) {
const char* string = &vm_symbol_bodies[0];
for (vmSymbolsIterator it = vmSymbolsRange.begin(); it != vmSymbolsRange.end(); ++it) {
vmSymbolID index = *it;
for (vmSymbolID index : EnumRange<vmSymbolID>{}) {
Symbol* sym = SymbolTable::new_permanent_symbol(string);
Symbol::_vm_symbols[as_int(index)] = sym;
string += strlen(string); // skip string body
Expand Down Expand Up @@ -113,12 +112,11 @@ void vmSymbols::initialize(TRAPS) {

#ifdef ASSERT
// Check for duplicates:
for (vmSymbolsIterator it1 = vmSymbolsRange.begin(); it1 != vmSymbolsRange.end(); ++it1) {
vmSymbolID i1 = *it1;

for (vmSymbolID i1 : EnumRange<vmSymbolID>{}) {
Symbol* sym = symbol_at(i1);
for (vmSymbolsIterator it2 = vmSymbolsRange.begin(); it2 != it1; ++it2) {
vmSymbolID i2 = *it2;
if (symbol_at(i2) == sym) {
for (vmSymbolID i2 : EnumRange<vmSymbolID>{vmSymbolID::FIRST_SID, i1}) {
if (i2 != i1 && symbol_at(i2) == sym) {
tty->print("*** Duplicate VM symbol SIDs %s(%d) and %s(%d): \"",
vm_symbol_enum_name(i2), as_int(i2),
vm_symbol_enum_name(i1), as_int(i1));
Expand All @@ -131,8 +129,7 @@ void vmSymbols::initialize(TRAPS) {

// Create an index for find_id:
{
for (vmSymbolsIterator it = vmSymbolsRange.begin(); it != vmSymbolsRange.end(); ++it) {
vmSymbolID index = *it;
for (vmSymbolID index : EnumRange<vmSymbolID>{}) {
vm_symbol_index[as_int(index)] = index;
}
int num_sids = SID_LIMIT-FIRST_SID;
Expand All @@ -153,8 +150,7 @@ void vmSymbols::initialize(TRAPS) {
assert(symbol_at(sid) == jlo, "");

// Make sure find_sid produces the right answer in each case.
for (vmSymbolsIterator it = vmSymbolsRange.begin(); it != vmSymbolsRange.end(); ++it) {
vmSymbolID index = *it;
for (vmSymbolID index : EnumRange<vmSymbolID>{}) {
Symbol* sym = symbol_at(index);
sid = find_sid(sym);
assert(sid == index, "symbol index works");
Expand All @@ -178,8 +174,7 @@ const char* vmSymbols::name_for(vmSymbolID sid) {
if (sid == vmSymbolID::NO_SID)
return "NO_SID";
const char* string = &vm_symbol_bodies[0];
for (vmSymbolsIterator it = vmSymbolsRange.begin(); it != vmSymbolsRange.end(); ++it) {
vmSymbolID index = *it;
for (vmSymbolID index : EnumRange<vmSymbolID>{}) {
if (index == sid)
return string;
string += strlen(string); // skip string body
Expand All @@ -192,8 +187,7 @@ const char* vmSymbols::name_for(vmSymbolID sid) {


void vmSymbols::symbols_do(SymbolClosure* f) {
for (vmSymbolsIterator it = vmSymbolsRange.begin(); it != vmSymbolsRange.end(); ++it) {
vmSymbolID index = *it;
for (vmSymbolID index : EnumRange<vmSymbolID>{}) {
f->do_symbol(&Symbol::_vm_symbols[as_int(index)]);
}
for (int i = 0; i < T_VOID+1; i++) {
Expand All @@ -202,8 +196,7 @@ void vmSymbols::symbols_do(SymbolClosure* f) {
}

void vmSymbols::metaspace_pointers_do(MetaspaceClosure *closure) {
for (vmSymbolsIterator it = vmSymbolsRange.begin(); it != vmSymbolsRange.end(); ++it) {
vmSymbolID index = *it;
for (vmSymbolID index : EnumRange<vmSymbolID>{}) {
closure->push(&Symbol::_vm_symbols[as_int(index)]);
}
for (int i = 0; i < T_VOID+1; i++) {
Expand Down Expand Up @@ -281,8 +274,7 @@ vmSymbolID vmSymbols::find_sid(const Symbol* symbol) {
// Make sure this is the right answer, using linear search.
// (We have already proven that there are no duplicates in the list.)
vmSymbolID sid2 = vmSymbolID::NO_SID;
for (vmSymbolsIterator it = vmSymbolsRange.begin(); it != vmSymbolsRange.end(); ++it) {
vmSymbolID index = *it;
for (vmSymbolID index : EnumRange<vmSymbolID>{}) {
Symbol* sym2 = symbol_at(index);
if (sym2 == symbol) {
sid2 = index;
Expand Down
2 changes: 0 additions & 2 deletions src/hotspot/share/classfile/vmSymbols.hpp
Expand Up @@ -722,8 +722,6 @@ enum class vmSymbolID : int {
};

ENUMERATOR_RANGE(vmSymbolID, vmSymbolID::FIRST_SID, vmSymbolID::LAST_SID)
constexpr EnumRange<vmSymbolID> vmSymbolsRange; // the default range of all valid vmSymbolIDs
using vmSymbolsIterator = EnumIterator<vmSymbolID>; // convenience

class vmSymbols: AllStatic {
friend class vmIntrinsics;
Expand Down
5 changes: 2 additions & 3 deletions src/hotspot/share/jfr/recorder/checkpoint/types/jfrType.cpp
Expand Up @@ -138,11 +138,10 @@ static const char* flag_value_origin_to_string(JVMFlagOrigin origin) {
}

void FlagValueOriginConstant::serialize(JfrCheckpointWriter& writer) {
constexpr EnumRange<JVMFlagOrigin> range;
constexpr EnumRange<JVMFlagOrigin> range{};
writer.write_count(static_cast<u4>(range.size()));

for (EnumIterator<JVMFlagOrigin> it = range.begin(); it != range.end(); ++it) {
JVMFlagOrigin origin = *it;
for (JVMFlagOrigin origin : range) {
writer.write_key(static_cast<u4>(origin));
writer.write(flag_value_origin_to_string(origin));
}
Expand Down
3 changes: 1 addition & 2 deletions src/hotspot/share/opto/compile.cpp
Expand Up @@ -234,8 +234,7 @@ void Compile::print_intrinsic_statistics() {
if (total == 0) total = 1; // avoid div0 in case of no successes
#define PRINT_STAT_LINE(name, c, f) \
tty->print_cr(" %4d (%4.1f%%) %s (%s)", (int)(c), ((c) * 100.0) / total, name, f);
for (vmIntrinsicsIterator it = vmIntrinsicsRange.begin(); it != vmIntrinsicsRange.end(); ++it) {
vmIntrinsicID id = *it;
for (vmIntrinsicID id : EnumRange<vmIntrinsicID>{}) {
int flags = _intrinsic_hist_flags[as_int(id)];
juint count = _intrinsic_hist_count[as_int(id)];
if ((flags | count) != 0) {
Expand Down
18 changes: 8 additions & 10 deletions src/hotspot/share/utilities/enumIterator.hpp
Expand Up @@ -57,21 +57,19 @@
// EnumRange -- defines the range of *one specific* iteration loop.
// EnumIterator -- the current point in the iteration loop.

// Example (see vmSymbols.hpp/cpp)
// Example:
//
// ENUMERATOR_RANGE(vmSymbolID, vmSymbolID::FIRST_SID, vmSymbolID::LAST_SID)
// constexpr EnumRange<vmSymbolID> vmSymbolsRange;
// using vmSymbolsIterator = EnumIterator<vmSymbolID>;
// /* With range-base for (recommended) */
// for (vmSymbolID index : EnumRange<vmSymbolID>{}) {
// ....
// }
//
// /* Without range-based for, allowed */
// /* Without range-based for */
// constexpr EnumRange<vmSymbolID> vmSymbolsRange{};
// using vmSymbolsIterator = EnumIterator<vmSymbolID>;
// for (vmSymbolsIterator it = vmSymbolsRange.begin(); it != vmSymbolsRange.end(); ++it) {
// vmSymbolID index = *it; ....
// }
//
// /* With range-base for, not allowed by HotSpot coding style yet */
// for (vmSymbolID index : vmSymbolsRange) {
// ....
// }

// EnumeratorRange is a traits type supporting iteration over the enumerators of T.
// Specializations must provide static const data members named "_start" and "_end".
Expand Down
68 changes: 68 additions & 0 deletions test/hotspot/gtest/utilities/test_enumIterator.cpp
Expand Up @@ -110,3 +110,71 @@ TEST(TestEnumIterator, implicit_iterator) {
}
EXPECT_EQ(it, range.end());
}

TEST(TestEnumIterator, explict_range_based_for_loop_full) {
int i = explicit_start;
for (ExplicitTest value : EnumRange<ExplicitTest>{}) {
EXPECT_EQ(size_t(i - explicit_start), EnumRange<ExplicitTest>{}.index(value));
EXPECT_TRUE(value == ExplicitTest::value1 ||
value == ExplicitTest::value2 ||
value == ExplicitTest::value3);
++i;
}
}

TEST(TestEnumIterator, explict_range_based_for_loop_start) {
constexpr EnumRange<ExplicitTest> range{ExplicitTest::value2};
int start = explicit_start + 2;
int i = start;
for (ExplicitTest value : range) {
EXPECT_EQ(size_t(i - start), range.index(value));
EXPECT_TRUE(value == ExplicitTest::value2 || value == ExplicitTest::value3);
EXPECT_TRUE(value != ExplicitTest::value1);
++i;
}
}

TEST(TestEnumIterator, explict_range_based_for_loop_start_end) {
constexpr EnumRange<ExplicitTest> range{ExplicitTest::value1, ExplicitTest::value2};
int start = explicit_start + 1;
int i = start;
for (ExplicitTest value : range) {
EXPECT_EQ(size_t(i - start), range.index(value));
EXPECT_TRUE(value == ExplicitTest::value1 || value == ExplicitTest::value2);
EXPECT_TRUE(value != ExplicitTest::value3);
++i;
}
}

TEST(TestEnumIterator, implicit_range_based_for_loop) {
int i = implicit_start;
for (ImplicitTest value : EnumRange<ImplicitTest>{}) {
EXPECT_EQ(size_t(i - implicit_start), EnumRange<ImplicitTest>{}.index(value));
++i;
}
}

TEST(TestEnumIterator, implicit_range_based_for_loop_start) {
int start = implicit_start + 1;
EnumRange<ImplicitTest> range{static_cast<ImplicitTest>(start)};
int i = start;
for (ImplicitTest value : range) {
EXPECT_EQ(size_t(i - start), range.index(value));
int iv = static_cast<int>(value);
EXPECT_TRUE(start <= iv && iv <= implicit_end);
++i;
}
}

TEST(TestEnumIterator, implicit_range_based_for_loop_start_end) {
int start = implicit_start + 1;
int end = implicit_end - 1;
EnumRange<ImplicitTest> range{static_cast<ImplicitTest>(start), static_cast<ImplicitTest>(end)};
int i = start;
for (ImplicitTest value : range) {
EXPECT_EQ(size_t(i - start), range.index(value));
int iv = static_cast<int>(value);
EXPECT_TRUE(start <= iv && iv <= end);
++i;
}
}

1 comment on commit 80dac5a

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

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

Please sign in to comment.