Skip to content

Commit 383a55b

Browse files
elbenoelbeno
committed
🎨 Refactor string catalog generation
Problem: - Ad-hoc data structures in `gen_str_catalog.py` are unwieldy. - Enum lookup was broken by encoding change with `encode_32` etc. Solution: - Introduction python classes for `Message` and `Module`; simplify code. - Add some tests for `gen_str_catalog.py`. - Fix enum lookup: include enum info inside `encode_32<T>`.
1 parent 3644dba commit 383a55b

File tree

7 files changed

+356
-194
lines changed

7 files changed

+356
-194
lines changed

‎.github/workflows/unit_tests.yml‎

Lines changed: 32 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -128,13 +128,6 @@ jobs:
128128
${{ matrix.install }}
129129
sudo apt install -y ninja-build python3-venv python3-pip
130130
131-
- name: Install python requirements for string catalog
132-
run: |
133-
python3 -m venv ${{github.workspace}}/test_venv
134-
source ${{github.workspace}}/test_venv/bin/activate
135-
pip install -r ${{github.workspace}}/tools/requirements.txt
136-
echo "${{github.workspace}}/test_venv/bin" >> $GITHUB_PATH
137-
138131
- name: Restore CPM cache
139132
env:
140133
cache-name: cpm-cache-0
@@ -161,6 +154,14 @@ jobs:
161154
path: ~/cpm-cache
162155
key: ${{runner.os}}-${{env.cache-name}}-${{ hashFiles('**/CMakeLists.txt', 'cmake/**') }}
163156

157+
- name: Install python requirements
158+
run: |
159+
python3 -m venv ${{github.workspace}}/test_venv
160+
source ${{github.workspace}}/test_venv/bin/activate
161+
pip install -r ${{github.workspace}}/tools/requirements.txt
162+
pip install -r ${{github.workspace}}/requirements.txt
163+
echo "${{github.workspace}}/test_venv/bin" >> $GITHUB_PATH
164+
164165
- name: Build Unit Tests
165166
run: cmake --build ${{github.workspace}}/build --config ${{matrix.build_type}} -v -t build_unit_tests
166167

@@ -214,13 +215,6 @@ jobs:
214215
${{ matrix.install }}
215216
sudo apt install -y ninja-build python3-venv python3-pip
216217
217-
- name: Install python requirements for string catalog
218-
run: |
219-
python3 -m venv ${{github.workspace}}/test_venv
220-
source ${{github.workspace}}/test_venv/bin/activate
221-
pip install -r ${{github.workspace}}/tools/requirements.txt
222-
echo "${{github.workspace}}/test_venv/bin" >> $GITHUB_PATH
223-
224218
- name: Restore CPM cache
225219
env:
226220
cache-name: cpm-cache-0
@@ -247,6 +241,14 @@ jobs:
247241
path: ~/cpm-cache
248242
key: ${{runner.os}}-${{env.cache-name}}-${{ hashFiles('**/CMakeLists.txt', 'cmake/**') }}
249243

244+
- name: Install python requirements
245+
run: |
246+
python3 -m venv ${{github.workspace}}/test_venv
247+
source ${{github.workspace}}/test_venv/bin/activate
248+
pip install -r ${{github.workspace}}/tools/requirements.txt
249+
pip install -r ${{github.workspace}}/requirements.txt
250+
echo "${{github.workspace}}/test_venv/bin" >> $GITHUB_PATH
251+
250252
- name: Build Unit Tests
251253
run: cmake --build ${{github.workspace}}/build --config ${{matrix.build_type}} -v -t build_unit_tests
252254

@@ -341,13 +343,6 @@ jobs:
341343
${{ matrix.install }}
342344
sudo apt install -y ninja-build python3-venv python3-pip
343345
344-
- name: Install python requirements for string catalog
345-
run: |
346-
python3 -m venv ${{github.workspace}}/test_venv
347-
source ${{github.workspace}}/test_venv/bin/activate
348-
pip install -r ${{github.workspace}}/tools/requirements.txt
349-
echo "${{github.workspace}}/test_venv/bin" >> $GITHUB_PATH
350-
351346
- name: Restore CPM cache
352347
env:
353348
cache-name: cpm-cache-0
@@ -382,6 +377,14 @@ jobs:
382377
# using leading to random crashes: https://reviews.llvm.org/D148280
383378
run: sudo sysctl vm.mmap_rnd_bits=28
384379

380+
- name: Install python requirements
381+
run: |
382+
python3 -m venv ${{github.workspace}}/test_venv
383+
source ${{github.workspace}}/test_venv/bin/activate
384+
pip install -r ${{github.workspace}}/tools/requirements.txt
385+
pip install -r ${{github.workspace}}/requirements.txt
386+
echo "${{github.workspace}}/test_venv/bin" >> $GITHUB_PATH
387+
385388
- name: Build Unit Tests
386389
run: cmake --build ${{github.workspace}}/build -t unit_tests
387390

@@ -394,13 +397,6 @@ jobs:
394397
run: |
395398
sudo apt update && sudo apt install -y gcc-${{env.DEFAULT_GCC_VERSION}} g++-${{env.DEFAULT_GCC_VERSION}} ninja-build python3-venv python3-pip valgrind
396399
397-
- name: Install python requirements for string catalog
398-
run: |
399-
python3 -m venv ${{github.workspace}}/test_venv
400-
source ${{github.workspace}}/test_venv/bin/activate
401-
pip install -r ${{github.workspace}}/tools/requirements.txt
402-
echo "${{github.workspace}}/test_venv/bin" >> $GITHUB_PATH
403-
404400
- name: Restore CPM cache
405401
env:
406402
cache-name: cpm-cache-0
@@ -427,6 +423,14 @@ jobs:
427423
path: ~/cpm-cache
428424
key: ${{runner.os}}-${{env.cache-name}}-${{ hashFiles('**/CMakeLists.txt', 'cmake/**') }}
429425

426+
- name: Install python requirements
427+
run: |
428+
python3 -m venv ${{github.workspace}}/test_venv
429+
source ${{github.workspace}}/test_venv/bin/activate
430+
pip install -r ${{github.workspace}}/tools/requirements.txt
431+
pip install -r ${{github.workspace}}/requirements.txt
432+
echo "${{github.workspace}}/test_venv/bin" >> $GITHUB_PATH
433+
430434
- name: Build Unit Tests
431435
run: cmake --build ${{github.workspace}}/build -t build_unit_tests
432436

‎include/log/catalog/catalog.hpp‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ using module_id = std::uint32_t;
1616
template <typename> extern auto catalog() -> string_id;
1717
template <typename> extern auto module() -> module_id;
1818

19-
struct encode_32;
20-
struct encode_64;
21-
struct encode_u32;
22-
struct encode_u64;
19+
template <typename> struct encode_32;
20+
template <typename> struct encode_64;
21+
template <typename> struct encode_u32;
22+
template <typename> struct encode_u64;

‎include/log/catalog/mipi_builder.hpp‎

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -16,39 +16,33 @@
1616

1717
namespace logging::mipi {
1818
template <typename T>
19-
concept signed_packable =
20-
std::signed_integral<T> and sizeof(T) <= sizeof(std::int64_t);
19+
concept signed_packable = std::signed_integral<stdx::underlying_type_t<T>> and
20+
sizeof(T) <= sizeof(std::int64_t);
2121

2222
template <typename T>
2323
concept unsigned_packable =
24-
std::unsigned_integral<T> and sizeof(T) <= sizeof(std::int64_t);
24+
std::unsigned_integral<stdx::underlying_type_t<T>> and
25+
sizeof(T) <= sizeof(std::int64_t);
2526

2627
template <typename T>
27-
concept enum_packable = std::is_enum_v<T> and sizeof(T) <= sizeof(std::int32_t);
28-
29-
template <typename T>
30-
concept packable =
31-
signed_packable<T> or unsigned_packable<T> or enum_packable<T>;
28+
concept packable = signed_packable<T> or unsigned_packable<T>;
3229

3330
template <typename T> struct encoding;
3431

3532
template <signed_packable T> struct encoding<T> {
3633
using encode_t = stdx::conditional_t<sizeof(T) <= sizeof(std::int32_t),
37-
encode_32, encode_64>;
34+
encode_32<T>, encode_64<T>>;
3835
using pack_t = stdx::conditional_t<sizeof(T) <= sizeof(std::int32_t),
3936
std::int32_t, std::int64_t>;
4037
};
4138

4239
template <unsigned_packable T> struct encoding<T> {
4340
using encode_t = stdx::conditional_t<sizeof(T) <= sizeof(std::int32_t),
44-
encode_u32, encode_u64>;
41+
encode_u32<T>, encode_u64<T>>;
4542
using pack_t = stdx::conditional_t<sizeof(T) <= sizeof(std::uint32_t),
4643
std::uint32_t, std::uint64_t>;
4744
};
4845

49-
template <enum_packable T>
50-
struct encoding<T> : encoding<stdx::underlying_type_t<T>> {};
51-
5246
template <packable T> using pack_as_t = typename encoding<T>::pack_t;
5347
template <packable T> using encode_as_t = typename encoding<T>::encode_t;
5448

‎test/log/encoder.cpp‎

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -146,17 +146,18 @@ TEST_CASE("argument packing", "[mipi]") {
146146
}
147147

148148
TEST_CASE("argument encoding", "[mipi]") {
149+
static_assert(std::same_as<logging::mipi::encode_as_t<std::int32_t>,
150+
encode_32<std::int32_t>>);
151+
static_assert(std::same_as<logging::mipi::encode_as_t<std::uint32_t>,
152+
encode_u32<std::uint32_t>>);
153+
static_assert(std::same_as<logging::mipi::encode_as_t<std::int64_t>,
154+
encode_64<std::int64_t>>);
155+
static_assert(std::same_as<logging::mipi::encode_as_t<std::uint64_t>,
156+
encode_u64<std::uint64_t>>);
149157
static_assert(
150-
std::same_as<logging::mipi::encode_as_t<std::int32_t>, encode_32>);
151-
static_assert(
152-
std::same_as<logging::mipi::encode_as_t<std::uint32_t>, encode_u32>);
153-
static_assert(
154-
std::same_as<logging::mipi::encode_as_t<std::int64_t>, encode_64>);
155-
static_assert(
156-
std::same_as<logging::mipi::encode_as_t<std::uint64_t>, encode_u64>);
157-
static_assert(std::same_as<logging::mipi::encode_as_t<char>, encode_32>);
158-
static_assert(
159-
std::same_as<logging::mipi::encode_as_t<unsigned char>, encode_u32>);
158+
std::same_as<logging::mipi::encode_as_t<char>, encode_32<char>>);
159+
static_assert(std::same_as<logging::mipi::encode_as_t<unsigned char>,
160+
encode_u32<unsigned char>>);
160161
}
161162

162163
TEST_CASE("log zero arguments", "[mipi]") {

‎tools/CMakeLists.txt‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1 +1,3 @@
11
mypy_lint(FILES gen_str_catalog.py)
2+
3+
add_unit_test("gen_str_catalog_test" PYTEST FILES "gen_str_catalog_test.py")

0 commit comments

Comments
 (0)