staniek requested changes to this revision. staniek added a comment. This revision now requires changes to proceed.
Thanks, lots of other people also touched this code but I tried this time to review :) I propose to separate the string literal changes. Also I wonder why there's "Context not available." error in this review, was the patch delivered using arc tools? INLINE COMMENTS > MsooXmlDrawingReaderTableImpl.h:58 > > - m_table = new KoTable; > - This changes life time > MsooXmlDrawingReaderTableMethods.h:40 > > - KoTable* m_table; > + ScopeKoTable m_table = ScopeKoTable(new KoTable); > QString m_currentTableName; This is declaration-only header, so better no, otherwise allocation of a new KoTable is added to all places we include the header, e.g. also PptxXmlSlideReader and in the future. > KoTable.h:112 > > +typedef QSharedPointer<KoTable> ScopeKoTable; > + Good idea, can work but I would propose to more consistent naming, e.g. like from KOSTYLE_DECLARE_SHARED_POINTER: typedef QSharedPointer<KoTable> Ptr; (inside of KoTable decl) > DocxXmlDocumentReader.cpp:5446 > > - KoTable table; > - m_table = &table; HERE1 > DocxXmlDocumentReader.cpp:6082 > + ScopeKoTable currentTable = m_table; > + m_table = ScopeKoTable(new KoTable); > int currentRow = m_currentTableRowNumber; Isn't this change in logic? > DocxXmlDocumentReader.h:258 > > - KoTable* m_table; > + ScopeKoTable m_table = ScopeKoTable(new KoTable); > QString m_currentTableStyleName; No code in declaration-only headers please, implementation may be moved to "HERE1" below. This way you won't need the extra #include here too. REPOSITORY R8 Calligra REVISION DETAIL https://phabricator.kde.org/D15412 To: anthonyfieroni, danders, boemann, #calligra:_3.0, staniek Cc: staniek, Calligra-Devel-list, dcaliste, cochise, vandenoever
