D15412: [filters] Extend table lifetime

[Jarosław Staniek]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/calligra-devel/attachments/20180913/c36c4837/attachment.htm>