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 ↗ | (On Diff #41377) | 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 ↗ | (On Diff #41377) | 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 ↗ | (On Diff #41377) | Isn't this change in logic? |
filters/words/docx/import/DocxXmlDocumentReader.h | ||
258 ↗ | (On Diff #41377) | 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. |
It can be allocated only in read_tbl but as you can see in bug report, use-after-free is very common to happen.
filters/libmsooxml/MsooXmlDrawingReaderTableMethods.h | ||
---|---|---|
40 ↗ | (On Diff #41377) | That isn't a problem, it's initialized at constructor time. |
filters/words/docx/import/DocxXmlDocumentReader.cpp | ||
6082 ↗ | (On Diff #41377) | No |