Turn EventDescription into class #340
Conversation
teto519f
force-pushed
the
issue-224-EventDescription-into-class
branch
from
36ca29e
to
e66d04a
Compare
include/lo2s/perf/reader.hpp
Outdated
| * Contains an opened instance of PerfEvent. | ||
| * Use PerfEvent.open() to construct an object | ||
| */ | ||
| class PerfEventInstance |
There was a problem hiding this comment.
I don't like the Instance postfix for a class name. Nobody does that. In C++ we would call these things "guard", like std::lock_guard. Hence, I propose EventGuard instead.
There was a problem hiding this comment.
Also, I have the feeling, that it might be good to have a template argument to this class, which is the actual event used, e.g., EventGuard<TracepointEvent> event_;.
There was a problem hiding this comment.
But the type of Event in EventGuard isn't being checked anywhere. Is there even a use for this information in EventGuard?
teto519f
force-pushed
the
issue-224-EventDescription-into-class
branch
3 times, most recently
from
6e7e736
to
820daaa
Compare
| Event::Event() : name_("") | ||
| { | ||
| memset(&attr_, 0, sizeof(attr_)); | ||
| attr_.size = sizeof(attr_); | ||
| attr_.type = PERF_TYPE_RAW; | ||
|
|
||
| attr_.config = 0; | ||
| attr_.config1 = 0; | ||
|
|
||
| attr_.sample_period = 0; | ||
| attr_.exclude_kernel = 1; | ||
| } |
There was a problem hiding this comment.
I don't think a default constructed event should be possible.
There was a problem hiding this comment.
Why not? This way you can say that a reader holds an event without needing to know the specifics.
There was a problem hiding this comment.
Because of the unknown specifics, you end up with an object that cannot be used. It's not initialized, a constructor should never give an ususable object.
src/perf/event.cpp
Outdated
| attr_.exclude_kernel = 1; | ||
| } | ||
|
|
||
| Event::Event([[maybe_unused]] uint64_t addr, bool enable_on_exec) |
There was a problem hiding this comment.
this does not set the name_ I don't want Event instances without a name.
The real issue is that this constructor is used for some special task and should better be a factory function
There was a problem hiding this comment.
Which name should a time event get? Or in other words: From where should the event take its name?
There was a problem hiding this comment.
"Time" sounds as good to me as anything else.
teto519f
force-pushed
the
issue-224-EventDescription-into-class
branch
4 times, most recently
from
3601ca0
to
7798068
Compare
This addresses issue #224
This PR creates a PerfEvent and a PerfEventInstance class, which can handle every type of event in lo2s. It aims to remove redundant code for event creation and to simplify their handling.