Dependencies cleanup

[Tydik42]

Replace three unmaintained external libraries with internal solutions:

• frozen -> util::StaticMap (constexpr std::array)
• better_enums -> magic_enum + util::EnumToStr
• atomicbitvector -> util::AtomicBitVector

The enum naming conversion (kCamelCase <-> snake_case) is a workaround that may need rework. We need to agree on whether the current solution is acceptable, or whether we need to do something else, for example, native Python enums.

[github-actions]

clang-tidy made some suggestions

[ol-imorozko]

Provides a constexpr immutable map-like lookup structure

But data is public field so anyone can modify it, like

util::StaticMap<int, int, 2> m{{{ {1, 10}, {2, 20} }}};
m.data[0].second = 123;

So let's move data to a private section

[ol-imorozko]

we can enforce it with concepts:

template <std::equality_comparable Key, typename Value, std::size_t N>
class StaticMap {

[ol-imorozko]

we want our class to be map-like, but nobody stops us from providing duplicate keys:

constexpr util::StaticMap<int, int, 3> m{{{ {1, 10}, {1, 999}, {2, 20} }}};
// m.At(1) is 10

This is bad because the code looks like 1 maps to 999, but it doesn’t. The first match wins because you linearly scan and return on first hit.

Of course in perfect world nobody will initialize StaticMap with such data, but someone could make a typo and enforcing no duplicates could save hours of debugging.

I suggest to add a compile-time duplicate check and static_assert if duplicates exist, smth like this:

template <std::equality_comparable Key, typename Value, std::size_t N>
class StaticMap {
public:
    using Pair = std::pair<Key, Value>;
    using Storage = std::array<Pair, N>;

    constexpr explicit StaticMap(Storage data) : data_(std::move(data)) {
        static_assert(!HasDuplicateKeys(data_), "StaticMap contains duplicate keys");
    }
    //...
private:
    Storage data_;

    static consteval bool HasDuplicateKeys(Storage const& data) {
        for (std::size_t i = 0; i < N; ++i) {
            for (std::size_t j = i + 1; j < N; ++j) {
                if (data[i].first == data[j].first) return true;
            }
        }
        return false;
    }

[ol-imorozko]

note: consteval there forces that check to happen at compile time so things like this:

std::array<std::pair<int,int>, 2> runtime_data = ...;
util::StaticMap<int,int,2> maybe_good(runtime_data);

won't compile at all. But I guess it makes sense here since StaticMap is constexpr, a replacement for frozen::unordered_map so basically if it’s not known at compile time, it shouldn't exist.

[ol-imorozko]

hm, why is size and empty are not starting with lowercase letter? We have that rule it in our clang-format so that we could use things like range-based for without extra hassle?

[ol-imorozko]

Use auto const& [k, v] : data_ to avoid doing pair.first and pair.second

[ol-imorozko]

We already have Find so we can just do this:

        if (auto const* p = Find(key))
            return *p;
        throw std::out_of_range("StaticMap::At: key not found");

[ol-imorozko]

nit:
if you want, you can add some tests. Honestly this is quite simple class so they are probably not necessary, but small things like

TEST(StaticMapTest, FindAndAtWork) {
    constexpr StaticMap<int, int, 3> map{{{{{1, 10}, {2, 20}, {3, 30}}}}};

    auto const* p = map.Find(2);
    ASSERT_NE(p, nullptr);
    EXPECT_EQ(*p, 20);

    EXPECT_EQ(map.At(3), 30);
}

TEST(StaticMapTest, MissingKey) {
    constexpr StaticMap<int, int, 1> map{{{{{42, 100}}}}};

    EXPECT_EQ(map.Find(0), nullptr);
    EXPECT_THROW(map.At(0), std::out_of_range);
}

won't hurt

[ol-imorozko]

Well done!

Only one question: original code relied on value-initialization of std::atomic inside std::vector:

struct atomwrapper {
    std::atomic<T> _a;
    atomwrapper() : _a() { }
};
//...
std::vector<AtomicBlockType> data_;
//...
    atomic_bv_t(size_t N) : _size(N),
                            kNumBlocks((N + kBitsPerBlock - 1) / kBitsPerBlock) {
        data_.resize(kNumBlocks);
    }

That _a() is value-initialization, so initially we will have an array full of zeroes which is expected.

But here we have std::make_unique<std::atomic<BlockType>[]>(num_blocks_), which only default constructs std::atomic, so no value-initialization takes place

[ol-imorozko]

Oh, nevermind, I just found out that this behavior got changed in C++20 and now default initialization of `std::atomic` actually value-initializes

[ol-imorozko]

I'm not an expert, but chatgpt tells me that in order to use apache 2.0 we need to:

• include copyright
• include license
• state changes
• include notice

It suggests this:

/*
 * Based on atomic_bitvector by Erik Garrison
 * https://github.com/ekg/atomicbitvector
 *
 * Copyright 2019 Erik Garrison
 * Copyright 2019 Facebook, Inc. and its affiliates
 *
 * Modifications:
 *  - Removed iterator support
 *  - Replaced std::vector with std::unique_ptr<std::atomic[]>
 *  - Simplified API
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 */