[onert] Apply inline variable to EventWriter #14652
Conversation
This commit applies inline variable to class EventWriter. ONE-DCO-1.0-Signed-off-by: ragmani <[email protected]>
| @@ -128,7 +128,7 @@ class EventWriter | |||
| void flush(WriteFormat write_format); | |||
|
|
|||
| private: | |||
| static std::mutex _mutex; | |||
| static inline std::mutex _mutex; | |||
There was a problem hiding this comment.
@ragmani Is it OK to use static inline for mutex class variable? Doesn't it have possibility to inlining mutex object by compiler?
There was a problem hiding this comment.
Yes, it's perfectly fine to use static inline for a mutex class variable. The inline keyword in this context is used to ensure that the variable has a single definition across all translation units, which allows you to define it in a header file without causing multiple definition errors. It does not cause the mutex object itself to be "inlined" or optimized away in the way that function inlining might; it merely affects linkage. The mutex will still behave as expected for synchronization.
There was a problem hiding this comment.
I found sentence from your link at #14647: https://en.cppreference.com/w/cpp/language/inline
- An inline function or variable(since C++17) with external linkage (e.g. not declared static) has the following additional properties:
- It has the same address in every translation unit.
Doesn't it mean that variable with non-external linkage (static variable) can be different address?
There was a problem hiding this comment.
I found answer at https://en.cppreference.com/w/cpp/language/storage_duration#external_linkage
- Any of the following names declared at namespace scope have external linkage, unless they are declared in an unnamed namespace
- names of classes, their member functions, static data members (const or not), nested classes and enumerations, and functions first introduced with friend declarations inside class bodies;
There was a problem hiding this comment.
This feature is hard to understand based on compiler world...
There was a problem hiding this comment.
This feature is hard to understand based on compiler world...
The two types of variables(static class member or namespace-scope variable) can be declared as inline variables. If both variables associated with inline keyword are problematic, it can be expected that the compiler will generate errors about that.
I think there is no need to overthink it and we just follow the rule at https://en.cppreference.com/w/cpp/language/inline regardless of compiler world.
Because the meaning of the keyword inline for functions came to mean "multiple definitions are permitted" rather than "inlining is preferred" since C++98, that meaning was extended to variables.
There was a problem hiding this comment.
I have concern about static initialization order. If there are dependencies between the compilation units (EventWriter.cc or EventWriter.h) and something else, something may go wrong.
As I know, it is safe to use explicit call like https://isocpp.org/wiki/faq/ctors#static-init-order.
The change may change the initialization order during compilation.
My concern is out of this PR's scope.
There was a problem hiding this comment.
Inline variables (introduced in C++17) help avoid multiple definitions across translation units by ensuring a single instance is shared. Inline variables don't change the rules of static initialization. They only help ensure that there is a single definition across translation units. So, if some static variables are already safe from the initialization order fiasco, switching them to inline variables won't affect that safety.
|
@Samsung/one_onert PTAL |
This commit applies inline variable to class EventWriter.
ONE-DCO-1.0-Signed-off-by: ragmani [email protected]