> On Oct. 4, 2016, 9 p.m., Albert Astals Cid wrote: > > I honestly don't see the benefit > > > > You changed a delete by a reset, is there any benefit at all other than > > making the code harder to read? > > Oliver Sander wrote: > I see three benefits: > a) I can see by looking at the declaration of m_drawingEngine that it has > ownership semantics. IMO that's quite helpful when trying to understand new > code. > b) I don't have to call 'delete' (admittedly that's minor here, because > m_drawingEngine is deleted only once). > c) Have a look at https://git.reviewboard.kde.org/r/128858 . There I had > to introduce > > delete m_drawingEngine; > m_drawingEngine = new SmoothPathEngine( > m_currentDrawingToolElement ); > > With a unique_ptr this is simply > > m_drawingEngine = std::make_unique<SmoothPathEngine>( > m_currentDrawingToolElement ); > > Oliver Sander wrote: > Alternatively, the last line could be > > m_drawingEngine.reset( new SmoothPathEngine( > m_currentDrawingToolElement ) ); > > which makes the intention even clearer. > > Albert Astals Cid wrote: > The code doesn't certainly seems clearer to me, but probably i'm just > old. I'll let others that are actually developing the frameworks branch > decide.
I agree with Albert, tbh., to me the new code takes longer time to grok. Personally I think the conventions for member pointers in Qt and Qt-based applications are easier to grasp than smart pointer ownership semantics. And another minor personal pet peeve is that std:: APIs tend to be badly designed compared to Qt APIs (counter-intuitive naming, lots of abbreviations, some inconsistencies, and in general just not following these guidelines: http://people.mpi-inf.mpg.de/~jblanche/api-design.pdf ). In addition the std:: coding style is different from the Okular code style (e. g. snake_case). That small rant aside, I don't mind if it is merged if anyone else wants it in (but I'm not going to give it a shipit myself). I know I Qt is moving towards using and recommending using more of the standard library instead of Qt's own similar things, so I'll have to learn to love the standard library eventually. :-) - Martin Tobias Holmedahl ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/129099/#review99787 ----------------------------------------------------------- On Oct. 4, 2016, 8:33 p.m., Oliver Sander wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/129099/ > ----------------------------------------------------------- > > (Updated Oct. 4, 2016, 8:33 p.m.) > > > Review request for Okular. > > > Repository: okular > > > Description > ------- > > Because it has ownership semantics. > > The code would be even shorter with std::make_unique, but that is C++14. Is > that allowed in Okular? > > > Diffs > ----- > > ui/presentationwidget.h 69574d2 > ui/presentationwidget.cpp c16d616 > > Diff: https://git.reviewboard.kde.org/r/129099/diff/ > > > Testing > ------- > > > Thanks, > > Oliver Sander > >
