[Lldb-commits] [PATCH] D33674: Implementation of Intel(R) Processor Trace support for Linux

[Ravitheja Addepally via Phabricator via lldb-commits]

ravitheja marked 5 inline comments as done.
ravitheja added inline comments.


================
Comment at: source/Plugins/Process/Linux/ProcessorTrace.cpp:307
+
+Status ProcessorTraceMonitor::Destroy() {
+  Status error;
----------------
labath wrote:
> I'd like this work to be done in the destructor. Just like nobody should see a constructed-but-not-yet-initialized object, so the destroyed-but-not-destructed state should not exist. Then you don't need to worry about setting the state of the object to some sane values.
> 
> In fact, if you utilize std::unique_ptr capabilities, then you may not even need a destructor at all. Just define a class
> ```
> class munmap_delete {
>   size_t m_length;
> 
> public:
>   munmap_delete(size_t length) : m_length(length) {}
>   void operator()(void *ptr) { munmap(ptr, m_length); }
> };
> ```
> 
> and then you can just declare  `
> `std::unique_ptr<perf_event_mmap_page, munmap_delete> mmap_base(mmap_result, munmap_delete(mmap_size))`
> 
> This way, the allocation and deallocation code are close together, and you don't need to write yourself `// we allocated extra page` reminders.
> 
> Since we store m_aux in a ArrayRef already, this would require duplicating the pointer in the unique_ptr member. I am fine with that, but if you want to avoid it you can just have a getAux() member function which sythesizes the arrayref on demand.
I can get the desired effect, by moving this function to the destructor, but we won't be able to catch munmap errors. I guess logging should be fine ? right now we were able to report errors in munmap.


https://reviews.llvm.org/D33674