In release mode, Investigate counter update to fail in compile time if the counter being updated is not defined.

[yamingk]

Right now updating a counter that is not defined will cause memory corruption in release mode.

Slack Discuss: https://ebay-eng.slack.com/archives/C07JWRZGG4D/p1740761894559139

Some Discussion History:
From Yaming Kuang:
See code below, for debug build, if someone is updating counter that doesn't exist, it should be able to throw runtime assert error, line: 263.
For release build, it won't, because see line:275, there is no check, it is just pretending it got valid index (this index could be anything) and then try to increase it.

#define COUNTER_INCREMENT(group, name, ...)                                                                            \
    __VALIDATE_AND_EXECUTE(group, NamedCounter, counter_increment, name, __VA_ARGS__)


258 #ifndef NDEBUG
259 #define __VALIDATE_AND_EXECUTE(group, type, method, name, ...)                                                         \
260     {                                                                                                                  \
261         using namespace sisl;                                                                                          \
262         const auto index{METRIC_NAME_TO_INDEX(type, name)};                                                            \
263         if (index == std::numeric_limits< decltype(index) >::max()) {                                                  \
264             fprintf(stderr, "Metric name '%s' not registered yet but used\n", BOOST_PP_STRINGIZE(name));               \
265             fflush(stderr);                                                                                            \
266             assert(0);                                                                                                 \
267         }                                                                                                              \
268         ((group).m_impl_ptr->method(index, __VA_ARGS__));                                                              \
269     }
270 #else
271 #define __VALIDATE_AND_EXECUTE(group, type, method, name, ...)                                                         \
272     {                                                                                                                  \
273         using namespace sisl;                                                                                          \
274         const auto index{METRIC_NAME_TO_INDEX(type, name)};                                                            \
275         ((group).m_impl_ptr->method(index, __VA_ARGS__));                                                              \
276     }
277 #endif

[yamingk]

From Yaming Kuang
Still it seems only able to do it at runtime, not compile time, even we apply the same logic check from line: 263 ~ 267 for release build which is going to hurt the runtime performance, as metrics updating is almost everywhere.
Maybe it is worth for someone to spend sometime to see how this can work even the name doesn't exist in that sisl::type

253 #define METRIC_NAME_TO_INDEX(type, name)                                                                               \
254     (sisl::type< decltype(BOOST_PP_CAT(BOOST_PP_STRINGIZE(name), _tstr)) >::getInstance().get_index())

[yamingk]

Brian Szmyd
Feb 12th at 10:14 AM
I take that back
@Yaming Kuang
getInstance() will always return a MetricGroup I think regardless of it being registered:

162     static NamedCounter& getInstance() {
163         static NamedCounter instance{};
164         return instance;
165     }
166
167     void set_index(const uint64_t index) { m_Index = index; }
168     [[nodiscard]] uint64_t get_index() const { return m_Index; }
169     [[nodiscard]] constexpr const char* get_name() const { return m_Name; }
170
171 private:
172     NamedCounter() : m_Index{std::numeric_limits< uint64_t >::max()} {}
173
174     static constexpr char m_Name[sizeof...(elements) + 1] = {elements..., '\0'};
175     size_t m_Index;

and m_Index is set to size_t::max which should work if it’s never been set. Since this is not compile time set we can’t check it at compile time.
I think it may be possible to move the instantiation of the class to the REGISTER macro so if someone doesn’t call that we’ll get a linker error…similar to how logging will cause a link failure if you don’t call SISL_LOGGING_DEF(module) somewhere…