From 0101808407c7215bf24eb8e2947b1ab17fb14e99 Mon Sep 17 00:00:00 2001 From: Jarek Kobus Date: Wed, 20 Oct 2021 13:27:01 +0200 Subject: [PATCH] Refactor HighlightingResultReporter In some unlikely circumstances it may happen that HighlightingResultReporter might be destroyed before returning from the HighlightingResultReporter::start() function. This may lead to undefined behavior. Refactor the HighlightingResultReporter so that instead of using QRunnable subclass we define simple function returning the QFuture object directly. Change-Id: Ib833771a7e46e87c83d10b59ca056a0147fabe88 Reviewed-by: Christian Kandeler --- .../clangeditordocumentprocessor.cpp | 7 +- .../clanghighlightingresultreporter.cpp | 109 ++++++++---------- .../clanghighlightingresultreporter.h | 44 ++----- .../highlightingresultreporter-test.cpp | 34 ++---- 4 files changed, 64 insertions(+), 130 deletions(-) diff --git a/src/plugins/clangcodemodel/clangeditordocumentprocessor.cpp b/src/plugins/clangcodemodel/clangeditordocumentprocessor.cpp index 94a44085056..2461120d9af 100644 --- a/src/plugins/clangcodemodel/clangeditordocumentprocessor.cpp +++ b/src/plugins/clangcodemodel/clangeditordocumentprocessor.cpp @@ -37,6 +37,8 @@ #include #include +#include + #include #include #include @@ -271,10 +273,7 @@ void ClangEditorDocumentProcessor::updateHighlighting( emit ifdefedOutBlocksUpdated(documentRevision, skippedPreprocessorBlocks); m_semanticHighlighter.setHighlightingRunner( - [tokenInfos]() { - auto *reporter = new HighlightingResultReporter(tokenInfos); - return reporter->start(); - }); + [tokenInfos]() { return highlightResults(tokenInfos); }); m_semanticHighlighter.run(); } } diff --git a/src/plugins/clangcodemodel/clanghighlightingresultreporter.cpp b/src/plugins/clangcodemodel/clanghighlightingresultreporter.cpp index 03070208ee8..149fb30c8b7 100644 --- a/src/plugins/clangcodemodel/clanghighlightingresultreporter.cpp +++ b/src/plugins/clangcodemodel/clanghighlightingresultreporter.cpp @@ -25,12 +25,13 @@ #include "clanghighlightingresultreporter.h" +#include #include +#include #include +#include #include -#include - namespace { TextEditor::TextStyle toTextStyle(ClangBackEnd::HighlightingType type) @@ -161,78 +162,62 @@ TextEditor::HighlightingResult toHighlightingResult( return result; } -} // anonymous - -namespace ClangCodeModel { -namespace Internal { - -HighlightingResultReporter::HighlightingResultReporter( - const QVector &tokenInfos) - : m_tokenInfos(tokenInfos) +void highlightResultsImpl(QFutureInterface &fi, + const QVector &tokenInfos, + int chunkSize) { - m_chunksToReport.reserve(m_chunkSize + 1); -} - -void HighlightingResultReporter::reportChunkWise( - const TextEditor::HighlightingResult &highlightingResult) -{ - if (m_chunksToReport.size() >= m_chunkSize) { - if (m_flushRequested && highlightingResult.line != m_flushLine) { - reportAndClearCurrentChunks(); - } else if (!m_flushRequested) { - m_flushRequested = true; - m_flushLine = highlightingResult.line; - } - } - - m_chunksToReport.append(highlightingResult); -} - -void HighlightingResultReporter::reportAndClearCurrentChunks() -{ - m_flushRequested = false; - m_flushLine = 0; - - if (!m_chunksToReport.isEmpty()) { - reportResults(m_chunksToReport); - m_chunksToReport.erase(m_chunksToReport.begin(), m_chunksToReport.end()); - } -} - -void HighlightingResultReporter::setChunkSize(int chunkSize) -{ - m_chunkSize = chunkSize; -} - -void HighlightingResultReporter::run() -{ - run_internal(); - reportFinished(); -} - -void HighlightingResultReporter::run_internal() -{ - if (isCanceled()) + if (fi.isCanceled()) return; - using ClangBackEnd::HighlightingType; + QVector chunksToReport; + chunksToReport.reserve(chunkSize + 1); - for (const auto &tokenInfo : qAsConst(m_tokenInfos)) + using ClangBackEnd::HighlightingType; + bool flushRequested = false; + int flushLine = 0; + + auto reportAndClearCurrentChunks = [&] { + flushRequested = false; + flushLine = 0; + + if (!chunksToReport.isEmpty()) { + fi.reportResults(chunksToReport); + chunksToReport.erase(chunksToReport.begin(), chunksToReport.end()); + } + }; + + auto reportChunkWise = [&](const TextEditor::HighlightingResult &highlightingResult) { + if (chunksToReport.size() >= chunkSize) { + if (flushRequested && highlightingResult.line != flushLine) { + reportAndClearCurrentChunks(); + } else if (!flushRequested) { + flushRequested = true; + flushLine = highlightingResult.line; + } + } + + chunksToReport.append(highlightingResult); + }; + + for (const auto &tokenInfo : tokenInfos) reportChunkWise(toHighlightingResult(tokenInfo)); - if (isCanceled()) + if (fi.isCanceled()) return; reportAndClearCurrentChunks(); } -QFuture HighlightingResultReporter::start() +} // anonymous + +namespace ClangCodeModel { +namespace Internal { + +QFuture highlightResults( + const QVector &tokenInfos, + int chunkSize) { - this->setRunnable(this); - this->reportStarted(); - QFuture future = this->future(); - QThreadPool::globalInstance()->start(this, QThread::LowestPriority); - return future; + return Utils::runAsync(highlightResultsImpl, tokenInfos, chunkSize); } } // namespace Internal diff --git a/src/plugins/clangcodemodel/clanghighlightingresultreporter.h b/src/plugins/clangcodemodel/clanghighlightingresultreporter.h index b3755fd264c..4999a7d77f1 100644 --- a/src/plugins/clangcodemodel/clanghighlightingresultreporter.h +++ b/src/plugins/clangcodemodel/clanghighlightingresultreporter.h @@ -25,48 +25,18 @@ #pragma once -#include -#include -#include -#include +#include +#include -#include - -#include +namespace TextEditor { class HighlightingResult; } +namespace ClangBackEnd { class TokenInfoContainer; } namespace ClangCodeModel { namespace Internal { -class HighlightingResultReporter: - public QObject, - public QRunnable, - public QFutureInterface -{ - Q_OBJECT - -public: - HighlightingResultReporter(const QVector &tokenInfos); - - void setChunkSize(int chunkSize); - - QFuture start(); - -private: - void run() override; - void run_internal(); - - void reportChunkWise(const TextEditor::HighlightingResult &highlightingResult); - void reportAndClearCurrentChunks(); - -private: - QVector m_tokenInfos; - QVector m_chunksToReport; - - int m_chunkSize = 100; - - bool m_flushRequested = false; - int m_flushLine = 0; -}; +QFuture highlightResults( + const QVector &tokenInfos, + int chunkSize = 100); } // namespace Internal } // namespace ClangCodeModel diff --git a/tests/unit/unittest/highlightingresultreporter-test.cpp b/tests/unit/unittest/highlightingresultreporter-test.cpp index dd2001e02ff..f7651f5134c 100644 --- a/tests/unit/unittest/highlightingresultreporter-test.cpp +++ b/tests/unit/unittest/highlightingresultreporter-test.cpp @@ -43,7 +43,6 @@ using ClangBackEnd::Document; using ClangBackEnd::Documents; using ClangBackEnd::UnsavedFiles; using ClangBackEnd::ChunksReportedMonitor; -using ClangCodeModel::Internal::HighlightingResultReporter; namespace { @@ -87,9 +86,7 @@ QVector generateTokenInfos(uint count) TEST_F(HighlightingResultReporter, StartAndFinish) { - auto reporter = new ::HighlightingResultReporter(noTokenInfos()); - - auto future = reporter->start(); + auto future = ClangCodeModel::Internal::highlightResults(noTokenInfos()); future.waitForFinished(); ASSERT_THAT(future.isFinished(), true); @@ -97,9 +94,7 @@ TEST_F(HighlightingResultReporter, StartAndFinish) TEST_F(HighlightingResultReporter, ReportNothingIfNothingToReport) { - auto reporter = new ::HighlightingResultReporter(generateTokenInfos(0)); - - auto future = reporter->start(); + auto future = ClangCodeModel::Internal::highlightResults(generateTokenInfos(0)); ChunksReportedMonitor monitor(future); ASSERT_THAT(monitor.resultsReadyCounter(), 0L); @@ -107,10 +102,7 @@ TEST_F(HighlightingResultReporter, ReportNothingIfNothingToReport) TEST_F(HighlightingResultReporter, ReportSingleResultAsOneChunk) { - auto reporter = new ::HighlightingResultReporter(generateTokenInfos(1)); - reporter->setChunkSize(1); - - auto future = reporter->start(); + auto future = ClangCodeModel::Internal::highlightResults(generateTokenInfos(1), 1); ChunksReportedMonitor monitor(future); ASSERT_THAT(monitor.resultsReadyCounter(), 1L); @@ -118,11 +110,7 @@ TEST_F(HighlightingResultReporter, ReportSingleResultAsOneChunk) TEST_F(HighlightingResultReporter, ReportRestIfChunkSizeNotReached) { - auto reporter = new ::HighlightingResultReporter(generateTokenInfos(1)); - const int notReachedChunkSize = 100; - reporter->setChunkSize(notReachedChunkSize); - - auto future = reporter->start(); + auto future = ClangCodeModel::Internal::highlightResults(generateTokenInfos(1), 100); ChunksReportedMonitor monitor(future); ASSERT_THAT(monitor.resultsReadyCounter(), 1L); @@ -130,10 +118,7 @@ TEST_F(HighlightingResultReporter, ReportRestIfChunkSizeNotReached) TEST_F(HighlightingResultReporter, ReportChunksWithoutRest) { - auto reporter = new ::HighlightingResultReporter(generateTokenInfos(4)); - reporter->setChunkSize(1); - - auto future = reporter->start(); + auto future = ClangCodeModel::Internal::highlightResults(generateTokenInfos(4), 1); ChunksReportedMonitor monitor(future); ASSERT_THAT(monitor.resultsReadyCounter(), 2L); @@ -141,10 +126,7 @@ TEST_F(HighlightingResultReporter, ReportChunksWithoutRest) TEST_F(HighlightingResultReporter, ReportSingleChunkAndRest) { - auto reporter = new ::HighlightingResultReporter(generateTokenInfos(5)); - reporter->setChunkSize(2); - - auto future = reporter->start(); + auto future = ClangCodeModel::Internal::highlightResults(generateTokenInfos(5), 2); ChunksReportedMonitor monitor(future); ASSERT_THAT(monitor.resultsReadyCounter(), 2L); @@ -159,10 +141,8 @@ TEST_F(HighlightingResultReporter, ReportCompleteLines) TokenInfoContainer(1, 2, 1, types), TokenInfoContainer(2, 1, 1, types), }; - auto reporter = new ::HighlightingResultReporter(tokenInfos); - reporter->setChunkSize(1); - auto future = reporter->start(); + auto future = ClangCodeModel::Internal::highlightResults(tokenInfos, 1); ChunksReportedMonitor monitor(future); ASSERT_THAT(monitor.resultsReadyCounter(), 2L);