Changeset View
Standalone View
duchain/helpers.cpp
Show First 20 Lines • Show All 56 Lines • ▼ Show 20 Line(s) | |||||
57 | #include "types/indexedcontainer.h" | 57 | #include "types/indexedcontainer.h" | ||
58 | #include "kdevpythonversion.h" | 58 | #include "kdevpythonversion.h" | ||
59 | #include "expressionvisitor.h" | 59 | #include "expressionvisitor.h" | ||
60 | 60 | | |||
61 | using namespace KDevelop; | 61 | using namespace KDevelop; | ||
62 | 62 | | |||
63 | namespace Python { | 63 | namespace Python { | ||
64 | 64 | | |||
65 | QMap<IProject*, QVector<QUrl>> Helper::cachedCustomIncludes; | 65 | QMap<IProject*, QVector<QUrl>>& Helper::cachedCustomIncludes() { | ||
66 | QMap<IProject*, QVector<QUrl>> Helper::cachedSearchPaths; | 66 | static QMap<IProject*, QVector<QUrl>> value; | ||
67 | QVector<QUrl> Helper::projectSearchPaths; | 67 | return value; | ||
68 | QStringList Helper::dataDirs; | 68 | } | ||
69 | IndexedString Helper::documentationFile; | 69 | | ||
70 | DUChainPointer<TopDUContext> Helper::documentationFileContext = DUChainPointer<TopDUContext>(nullptr); | 70 | QMap<IProject*, QVector<QUrl>>& Helper::cachedSearchPaths() { | ||
71 | QStringList Helper::correctionFileDirs; | 71 | static QMap<IProject*, QVector<QUrl>> value; | ||
72 | QString Helper::localCorrectionFileDir; | 72 | return value; | ||
73 | QMutex Helper::cacheMutex; | 73 | } | ||
74 | QMutex Helper::projectPathLock; | 74 | | ||
75 | QVector<QUrl>& Helper::projectSearchPaths() { | ||||
76 | static QVector<QUrl> value; | ||||
77 | return value; | ||||
78 | } | ||||
79 | | ||||
80 | IndexedString& Helper::documentationFile() { | ||||
flherne: Is this necessary? The variable is only used in `getDataDirs()`. Some of the others are similar. | |||||
81 | static IndexedString value; | ||||
82 | return value; | ||||
83 | } | ||||
84 | | ||||
85 | DUChainPointer<TopDUContext>& Helper::documentationFileContext() { | ||||
86 | static DUChainPointer<TopDUContext> value = DUChainPointer<TopDUContext>(nullptr); | ||||
87 | return value; | ||||
88 | } | ||||
89 | | ||||
90 | QMutex& Helper::cacheMutex() { | ||||
91 | static QMutex value; | ||||
92 | return value; | ||||
93 | } | ||||
94 | | ||||
95 | QMutex& Helper::projectPathLock() { | ||||
96 | static QMutex value; | ||||
97 | return value; | ||||
98 | } | ||||
75 | 99 | | |||
76 | void Helper::scheduleDependency(const IndexedString& dependency, int betterThanPriority) | 100 | void Helper::scheduleDependency(const IndexedString& dependency, int betterThanPriority) | ||
77 | { | 101 | { | ||
78 | BackgroundParser* bgparser = KDevelop::ICore::self()->languageController()->backgroundParser(); | 102 | BackgroundParser* bgparser = KDevelop::ICore::self()->languageController()->backgroundParser(); | ||
79 | bool needsReschedule = true; | 103 | bool needsReschedule = true; | ||
80 | if ( bgparser->isQueued(dependency) ) { | 104 | if ( bgparser->isQueued(dependency) ) { | ||
81 | const auto priority= bgparser->priorityForDocument(dependency); | 105 | const auto priority= bgparser->priorityForDocument(dependency); | ||
Can these be next to the variable-containing-function they protect? Does it make sense for this to protect both cachedSearchPaths() and cachedCustomIncludes()? They don't seem particularly related. flherne: Can these be next to the variable-containing-function they protect?
Does it make sense for… | |||||
82 | if ( priority > betterThanPriority - 1 ) { | 106 | if ( priority > betterThanPriority - 1 ) { | ||
83 | bgparser->removeDocument(dependency); | 107 | bgparser->removeDocument(dependency); | ||
84 | } | 108 | } | ||
85 | else { | 109 | else { | ||
86 | needsReschedule = false; | 110 | needsReschedule = false; | ||
87 | } | 111 | } | ||
88 | } | 112 | } | ||
89 | if ( needsReschedule ) { | 113 | if ( needsReschedule ) { | ||
▲ Show 20 Lines • Show All 187 Lines • ▼ Show 20 Line(s) | 300 | if ( alias ) { | |||
277 | DUChainReadLocker lock; | 301 | DUChainReadLocker lock; | ||
278 | return alias->aliasedDeclaration().data(); | 302 | return alias->aliasedDeclaration().data(); | ||
279 | } | 303 | } | ||
280 | else | 304 | else | ||
281 | return decl; | 305 | return decl; | ||
282 | } | 306 | } | ||
283 | 307 | | |||
284 | QStringList Helper::getDataDirs() { | 308 | QStringList Helper::getDataDirs() { | ||
285 | if ( Helper::dataDirs.isEmpty() ) { | 309 | QMutexLocker lock(&cacheMutex()); | ||
This doesn't look thread-safe (in either the old or modified version); nothing locks the value while it's being written to. flherne: This doesn't look thread-safe (in either the old or modified version); nothing locks the value… | |||||
286 | Helper::dataDirs = QStandardPaths::locateAll(QStandardPaths::GenericDataLocation, "kdevpythonsupport/documentation_files", QStandardPaths::LocateDirectory); | 310 | static QStringList dataDirs = QStandardPaths::locateAll(QStandardPaths::GenericDataLocation, | ||
287 | } | 311 | "kdevpythonsupport/documentation_files", QStandardPaths::LocateDirectory); | ||
288 | return Helper::dataDirs; | 312 | return dataDirs; | ||
289 | } | 313 | } | ||
290 | 314 | | |||
291 | KDevelop::IndexedString Helper::getDocumentationFile() | 315 | KDevelop::IndexedString Helper::getDocumentationFile() | ||
292 | { | 316 | { | ||
293 | if ( Helper::documentationFile.isEmpty() ) { | 317 | QMutexLocker lock(&cacheMutex()); | ||
318 | if ( Helper::documentationFile().isEmpty() ) { | ||||
294 | auto path = QStandardPaths::locate(QStandardPaths::GenericDataLocation, "kdevpythonsupport/documentation_files/builtindocumentation.py"); | 319 | auto path = QStandardPaths::locate(QStandardPaths::GenericDataLocation, "kdevpythonsupport/documentation_files/builtindocumentation.py"); | ||
295 | Helper::documentationFile = IndexedString(path); | 320 | Helper::documentationFile() = IndexedString(path); | ||
flherne: ditto | |||||
296 | } | 321 | } | ||
297 | return Helper::documentationFile; | 322 | return Helper::documentationFile(); | ||
298 | } | 323 | } | ||
299 | 324 | | |||
300 | ReferencedTopDUContext Helper::getDocumentationFileContext() | 325 | ReferencedTopDUContext Helper::getDocumentationFileContext() | ||
301 | { | 326 | { | ||
302 | if ( Helper::documentationFileContext ) { | 327 | QMutexLocker lock(&cacheMutex()); | ||
303 | return ReferencedTopDUContext(Helper::documentationFileContext.data()); | 328 | if ( Helper::documentationFileContext() ) { | ||
329 | return ReferencedTopDUContext(Helper::documentationFileContext().data()); | ||||
why is this locking the cache mutex? shouldn't / couldn't you simply lock the duchain here instead? mwolff: why is this locking the cache mutex? shouldn't / couldn't you simply lock the duchain here… | |||||
304 | } | 330 | } | ||
305 | else { | 331 | else { | ||
306 | DUChainReadLocker lock; | 332 | DUChainReadLocker lock; | ||
307 | auto file = Helper::getDocumentationFile(); | 333 | auto file = Helper::getDocumentationFile(); | ||
308 | ReferencedTopDUContext ctx = ReferencedTopDUContext(DUChain::self()->chainForDocument(file)); | 334 | ReferencedTopDUContext ctx = ReferencedTopDUContext(DUChain::self()->chainForDocument(file)); | ||
309 | Helper::documentationFileContext = DUChainPointer<TopDUContext>(ctx.data()); | 335 | Helper::documentationFileContext() = DUChainPointer<TopDUContext>(ctx.data()); | ||
flherne: ditto | |||||
310 | return ctx; | 336 | return ctx; | ||
311 | } | 337 | } | ||
312 | } | 338 | } | ||
313 | 339 | | |||
314 | // stolen from KUrl. duh. | 340 | // stolen from KUrl. duh. | ||
315 | static QString _relativePath(const QString &base_dir, const QString &path) | 341 | static QString _relativePath(const QString &base_dir, const QString &path) | ||
316 | { | 342 | { | ||
317 | QString _base_dir(QDir::cleanPath(base_dir)); | 343 | QString _base_dir(QDir::cleanPath(base_dir)); | ||
Show All 25 Lines | |||||
343 | if ((level < list2.count()) && (path[path.length()-1] != QLatin1Char('/'))) | 369 | if ((level < list2.count()) && (path[path.length()-1] != QLatin1Char('/'))) | ||
344 | result.truncate(result.length()-1); | 370 | result.truncate(result.length()-1); | ||
345 | 371 | | |||
346 | return result; | 372 | return result; | ||
347 | } | 373 | } | ||
348 | 374 | | |||
349 | QUrl Helper::getCorrectionFile(const QUrl& document) | 375 | QUrl Helper::getCorrectionFile(const QUrl& document) | ||
350 | { | 376 | { | ||
351 | if ( Helper::correctionFileDirs.isEmpty() ) { | 377 | QMutexLocker lock(&cacheMutex()); | ||
352 | Helper::correctionFileDirs = QStandardPaths::locateAll(QStandardPaths::GenericDataLocation, | 378 | static auto correctionFileDirs = QStandardPaths::locateAll(QStandardPaths::GenericDataLocation, | ||
flherne: ditto | |||||
353 | "kdevpythonsupport/correction_files/", | 379 | "kdevpythonsupport/correction_files/", | ||
354 | QStandardPaths::LocateDirectory); | 380 | QStandardPaths::LocateDirectory); | ||
instead of using the mutex for this, initialize this static directly (that is guaranteed to be threadsafe with c++11): static const auto correctionFileDirs = QStandardPaths::locateAll(...); mwolff: instead of using the mutex for this, initialize this static directly (that is guaranteed to be… | |||||
355 | } | | |||
356 | | ||||
357 | foreach (QString correctionFileDir, correctionFileDirs) { | 381 | foreach (QString correctionFileDir, correctionFileDirs) { | ||
358 | foreach ( const QUrl& basePath, Helper::getSearchPaths(QUrl()) ) { | 382 | foreach ( const QUrl& basePath, Helper::getSearchPaths(QUrl()) ) { | ||
You gain mutex at line 377 here it's deadlock, QBasicMutex is not recursive. anthonyfieroni: You gain mutex at line 377 here it's deadlock, QBasicMutex is not recursive. | |||||
I don't believe this is truly safe. It prevents multiple threads writing simultaneously, but other threads may read while the write takes place, which isn't permitted. Either the mutex should cover the whole thing, or it might be more performant to use a QReadWriteLock? Of course, since there was no locking at all before and nothing blew up, this is probably a statistical irrelevance... flherne: I don't believe this is truly safe. It prevents multiple threads writing simultaneously, but… | |||||
359 | if ( ! basePath.isParentOf(document) ) { | 383 | if ( ! basePath.isParentOf(document) ) { | ||
360 | continue; | 384 | continue; | ||
361 | } | 385 | } | ||
362 | auto base = basePath.path(); | 386 | auto base = basePath.path(); | ||
363 | auto doc = document.path(); | 387 | auto doc = document.path(); | ||
364 | auto relative = _relativePath(base, doc); | 388 | auto relative = _relativePath(base, doc); | ||
365 | auto fullPath = correctionFileDir + "/" + relative; | 389 | auto fullPath = correctionFileDir + "/" + relative; | ||
366 | if ( QFile::exists(fullPath) ) { | 390 | if ( QFile::exists(fullPath) ) { | ||
367 | return QUrl::fromLocalFile(fullPath).adjusted(QUrl::NormalizePathSegments); | 391 | return QUrl::fromLocalFile(fullPath).adjusted(QUrl::NormalizePathSegments); | ||
368 | } | 392 | } | ||
369 | } | 393 | } | ||
370 | } | 394 | } | ||
371 | return {}; | 395 | return {}; | ||
372 | } | 396 | } | ||
373 | 397 | | |||
374 | QUrl Helper::getLocalCorrectionFile(const QUrl& document) | 398 | QUrl Helper::getLocalCorrectionFile(const QUrl& document) | ||
375 | { | 399 | { | ||
376 | if ( Helper::localCorrectionFileDir.isNull() ) { | 400 | QMutexLocker lock(&cacheMutex()); | ||
flherne: ditto... | |||||
mwolff: dito | |||||
377 | Helper::localCorrectionFileDir = QStandardPaths::writableLocation(QStandardPaths::GenericDataLocation) + QLatin1Char('/') + "kdevpythonsupport/correction_files/"; | 401 | static auto localCorrectionFileDir = QStandardPaths::writableLocation(QStandardPaths::GenericDataLocation) + QLatin1Char('/') + "kdevpythonsupport/correction_files/"; | ||
flherne: As above. | |||||
378 | } | | |||
379 | 402 | | |||
380 | auto absolutePath = QUrl(); | 403 | auto absolutePath = QUrl(); | ||
381 | foreach ( const auto& basePath, Helper::getSearchPaths(QUrl()) ) { | 404 | foreach ( const auto& basePath, Helper::getSearchPaths(QUrl()) ) { | ||
382 | if ( ! basePath.isParentOf(document) ) { | 405 | if ( ! basePath.isParentOf(document) ) { | ||
383 | continue; | 406 | continue; | ||
384 | } | 407 | } | ||
385 | auto path = QDir(basePath.path()).relativeFilePath(document.path()); | 408 | auto path = QDir(basePath.path()).relativeFilePath(document.path()); | ||
386 | absolutePath = QUrl::fromLocalFile(Helper::localCorrectionFileDir + path); | 409 | absolutePath = QUrl::fromLocalFile(localCorrectionFileDir + path); | ||
387 | break; | 410 | break; | ||
388 | } | 411 | } | ||
389 | return absolutePath; | 412 | return absolutePath; | ||
390 | } | 413 | } | ||
391 | 414 | | |||
392 | QString Helper::getPythonExecutablePath(IProject* project) | 415 | QString Helper::getPythonExecutablePath(IProject* project) | ||
393 | { | 416 | { | ||
394 | if ( project ) { | 417 | if ( project ) { | ||
▲ Show 20 Lines • Show All 52 Lines • ▼ Show 20 Line(s) | 443 | #ifdef Q_OS_WIN | |||
447 | } | 470 | } | ||
448 | #endif | 471 | #endif | ||
449 | // fallback | 472 | // fallback | ||
450 | return PYTHON_EXECUTABLE; | 473 | return PYTHON_EXECUTABLE; | ||
451 | } | 474 | } | ||
452 | 475 | | |||
453 | QVector<QUrl> Helper::getSearchPaths(const QUrl& workingOnDocument) | 476 | QVector<QUrl> Helper::getSearchPaths(const QUrl& workingOnDocument) | ||
454 | { | 477 | { | ||
455 | QMutexLocker lock(&Helper::cacheMutex); | 478 | QMutexLocker lock(&Helper::cacheMutex()); | ||
456 | QVector<QUrl> searchPaths; | 479 | QVector<QUrl> searchPaths; | ||
457 | // search in the projects, as they're packages and likely to be installed or added to PYTHONPATH later | 480 | // search in the projects, as they're packages and likely to be installed or added to PYTHONPATH later | ||
458 | // and also add custom include paths that are defined in the projects | 481 | // and also add custom include paths that are defined in the projects | ||
459 | 482 | | |||
460 | auto project = ICore::self()->projectController()->findProjectForUrl(workingOnDocument); | 483 | auto project = ICore::self()->projectController()->findProjectForUrl(workingOnDocument); | ||
this is still not thread safe due to calls like this. you should probably ensure this is only being called from the main thread. once you have done that, afaik you can remove the cacheMutex as it won't be needed anymore mwolff: this is still not thread safe due to calls like this. you should probably ensure this is only… | |||||
461 | { | 484 | { | ||
462 | QMutexLocker lock(&Helper::projectPathLock); | 485 | QMutexLocker lock(&Helper::projectPathLock()); | ||
463 | searchPaths << Helper::projectSearchPaths; | 486 | searchPaths << Helper::projectSearchPaths(); | ||
464 | searchPaths << Helper::cachedCustomIncludes.value(project); | 487 | searchPaths << Helper::cachedCustomIncludes().value(project); | ||
465 | } | 488 | } | ||
466 | 489 | | |||
467 | foreach ( const QString& path, getDataDirs() ) { | 490 | foreach ( const QString& path, getDataDirs() ) { | ||
468 | searchPaths.append(QUrl::fromLocalFile(path)); | 491 | searchPaths.append(QUrl::fromLocalFile(path)); | ||
469 | } | 492 | } | ||
470 | 493 | | |||
471 | if ( !cachedSearchPaths.contains(project) ) { | 494 | if ( !cachedSearchPaths().contains(project) ) { | ||
472 | QVector<QUrl> cachedForProject; | 495 | QVector<QUrl> cachedForProject; | ||
473 | qCDebug(KDEV_PYTHON_DUCHAIN) << "*** Collecting search paths..."; | 496 | qCDebug(KDEV_PYTHON_DUCHAIN) << "*** Collecting search paths..."; | ||
474 | QStringList getpath; | 497 | QStringList getpath; | ||
475 | getpath << "-c" << "import sys; sys.stdout.write('$|$'.join(sys.path))"; | 498 | getpath << "-c" << "import sys; sys.stdout.write('$|$'.join(sys.path))"; | ||
476 | 499 | | |||
477 | QProcess python; | 500 | QProcess python; | ||
478 | python.start(getPythonExecutablePath(project), getpath); | 501 | python.start(getPythonExecutablePath(project), getpath); | ||
479 | python.waitForFinished(1000); | 502 | python.waitForFinished(1000); | ||
Show All 10 Lines | 511 | else { | |||
490 | searchPaths.append(QUrl::fromLocalFile("/usr/lib/python" PYTHON_VERSION_STR)); | 513 | searchPaths.append(QUrl::fromLocalFile("/usr/lib/python" PYTHON_VERSION_STR)); | ||
491 | searchPaths.append(QUrl::fromLocalFile("/usr/lib/python" PYTHON_VERSION_STR "/site-packages")); | 514 | searchPaths.append(QUrl::fromLocalFile("/usr/lib/python" PYTHON_VERSION_STR "/site-packages")); | ||
492 | QString path = qgetenv("PYTHONPATH"); | 515 | QString path = qgetenv("PYTHONPATH"); | ||
493 | QStringList paths = path.split(':'); | 516 | QStringList paths = path.split(':'); | ||
494 | foreach ( const QString& path, paths ) { | 517 | foreach ( const QString& path, paths ) { | ||
495 | cachedForProject.append(QUrl::fromLocalFile(path)); | 518 | cachedForProject.append(QUrl::fromLocalFile(path)); | ||
496 | } | 519 | } | ||
497 | } | 520 | } | ||
498 | qCDebug(KDEV_PYTHON_DUCHAIN) << " *** Done. Got search paths: " << cachedSearchPaths; | 521 | qCDebug(KDEV_PYTHON_DUCHAIN) << " *** Done. Got search paths: " << cachedSearchPaths(); | ||
499 | cachedSearchPaths.insert(project, cachedForProject); | 522 | cachedSearchPaths().insert(project, cachedForProject); | ||
500 | } | 523 | } | ||
501 | 524 | | |||
502 | searchPaths.append(cachedSearchPaths.value(project)); | 525 | searchPaths.append(cachedSearchPaths().value(project)); | ||
503 | 526 | | |||
504 | auto dir = workingOnDocument.adjusted(QUrl::RemoveFilename); | 527 | auto dir = workingOnDocument.adjusted(QUrl::RemoveFilename); | ||
505 | if ( ! dir.isEmpty() ) { | 528 | if ( ! dir.isEmpty() ) { | ||
506 | // search in the current packages | 529 | // search in the current packages | ||
507 | searchPaths.append(dir); | 530 | searchPaths.append(dir); | ||
508 | } | 531 | } | ||
509 | 532 | | |||
510 | return searchPaths; | 533 | return searchPaths; | ||
▲ Show 20 Lines • Show All 50 Lines • Show Last 20 Lines |
Is this necessary? The variable is only used in getDataDirs(). Some of the others are similar.