CCBUG: 379255
Details
- Reviewers
danders boemann staniek - Group Reviewers
Calligra: 3.0 - Commits
- R8:cb7ff65d2e7c: Extend table lifetime
Concurrent calls on getPresetTable can lead to race condition but i can't see such a usage, it can declared thread_local in such cases
Diff Detail
- Repository
- R8 Calligra
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Aggressive refactor, it makes m_table a shared pointer, presetTables is one time initialized.
Sorry, don't have any qualified opinons, I don't quite understand the original code either, so...
Imo if you think it is good, land it.
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?
filters/libmsooxml/MsooXmlDrawingReaderTableImpl.h | ||
---|---|---|
58 | This changes life time | |
filters/libmsooxml/MsooXmlDrawingReaderTableMethods.h | ||
40 | 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. | |
filters/libodf2/KoTable.h | ||
112 | 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) | |
filters/words/docx/import/DocxXmlDocumentReader.cpp | ||
5446 | HERE1 | |
6082 | Isn't this change in logic? | |
filters/words/docx/import/DocxXmlDocumentReader.h | ||
258 | 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. |