Jump to conversation
Unresolved conversations (6)
@snehasish snehasish Nov 20, 2023
`addRecord` without a weight is a thin convenience wrapper, I think it's ok to drop this if condition in favour of directly passing ProfWeight.
llvm/unittests/ProfileData/InstrProfTest.cpp
mingmingl-llvm
@snehasish snehasish Nov 20, 2023
I would like to keep the test assertions/expectations in the test themselves, using this helper makes the tests harder to debug on failure. Here is a slightly different refactoring you can consider: These are already parameterized tests so lets extend the params to include endianness and weights. We can combine the params in INSTANTIATE_TEST_SUITE_P using ``` test::Combine(testing::Bool(), Values(TestParam{/*endian=*/little, /*weight=*/1}, TestParam{/*endian=*/little, /*weight=*/10}, TestParam{/*endian=*/big, /*weight=*/1} ) ) ``` Here TestParam is a new struct which is introduced. This will keep the existing coverage. You can also increase coverage by combining all cases (as cartesian product). ``` test::Combine(testing::Bool(), // sparse? Values(little, big), // endianness Values(1, 10), // weights ) ) ``` What do you think? Feel free to avoid if this is requires more time than you want to invest.
llvm/unittests/ProfileData/InstrProfTest.cpp
mingmingl-llvm snehasish
@snehasish snehasish Nov 20, 2023
Why is this needed now? It makes me think that there is some dependency across the tests..
llvm/unittests/ProfileData/InstrProfTest.cpp
mingmingl-llvm
@snehasish snehasish Nov 20, 2023
I think this should be an assertion since we shouldn't access the contents of R if this fails.
llvm/unittests/ProfileData/InstrProfTest.cpp
@david-xl david-xl Nov 20, 2023
Can this setup method be shared with other value profile kind?
llvm/unittests/ProfileData/InstrProfTest.cpp
mingmingl-llvm
@david-xl david-xl Nov 20, 2023
Can set default weight to 1.
Outdated
llvm/unittests/ProfileData/InstrProfTest.cpp
mingmingl-llvm
Resolved conversations (0)