Patch cleaned up, stripped the LevelDB and Kyoto backends that never satisfied me.
I did leave the original file-based storage backend, not because I think it has to be preserved if this ever gets in but to provide a quicker way to compare performance (and behaviour if ever someone testing this runs into issues).

Aaron's last suggestion made me realise I forget a few things when porting the patch back to using QSocketNotifier.
The shutdown procedure was intended to (and now does) include closing the signal pipe, verifying the descriptor before writing to it makes sense. So does handling failure there by re-raising the signal with the default handler.
I thought it would be even more robust to move the actual write into the if and check whether it succeeded, after all we want to be certain that these signals are always handled.

In D16218#346296, @rjvbb wrote:

Maybe you can add an else branch where you re-raise the signal with the default handler.

If the goal is just to let a 2nd signal trigger the default reaction from the runtime then it can be even simpler.

This adds a safety against a race-like condition I've seen happen once in KDevelop.
It may not be required to make m_mWin a QPointer but it seemed sensible to do.

Ah, yes, I'll have to remember to clean it up and keep only the LMDB backend.

Sorry, I don't see this going in in this or a similar form.

I'm a bit confused... I thought KDevelop had much more advanced plugins/features for this? What does KDevelop need this old plugin for? Non-C/C++ languages or what?

I'm a bit confused... I thought KDevelop had much more advanced plugins/features for this? What does KDevelop need this old plugin for? Non-C/C++ languages or what?

Merged in changes from an older attempt I forgot about (apologies!).

• Change test case and convert octal literal to decimal

• Add a message when the scratch list is empty

In D16484#351906, @amhndu wrote:

Would something like this be worth adding ?

Would something like this be worth adding ?

cpp
index 5c193432c9..1b5fb57352 100644
--- a/plugins/scratchpad/scratchpadview.cpp
+++ b/plugins/scratchpad/scratchpadview.cpp
@@ -37,6 +37,7 @@
 #include <QWidgetAction>
 #include <QLineEdit>
 #include <QInputDialog>
+#include <QPainter>

Marking inline comments done.

• Make commands per-config and new scratches use command set last for suffix. I could not use mime types, as I was having some problems with detecting them,
• Removed dependency of a compiler
• Some other minor improvements

Updated. Thanks! I will push it :)

please cleanup the test slightly and then push it directly (no need for another round of review)

• changed as requested, will push tomorrow or so

• Initial test, works with the patch, fails without :)

Thanks for the feedback, I've updated with some of the requested changes.

One thing comes to mind: you kind of require that there is a compiler, while 3 of the 4 languages KDevelop officially supports (PHP, Python and JS) do not use one. I think it would be nice if this plugin would also easily work for e.g. Python snippets.

That's one of the problems I've had, the command box coupled with your suggestion to have per-scratch config could help somewhat. Is there some kdev interface that would help here or some other solution ?

I'd have to use this for a while to see whether it works well in practice, but in general it seems like a cool idea.

Some improvements

... you want to add the feature

+1
Yes please...

In D15565#333758, @kossebau wrote:

In D15565#333756, @antonanikin wrote:

Is there a chance those aspects could be split out into separate patches?
Also still hoping for a variant of the bug fix patch which does not need a string freeze break, if possible.

Hi, Friedrich, sorry for delay. I think we should push it as is. Since current (wrong) logic of plugin controller we can have execute plugin unloaded with heaptrack loaded in same time. But execute plugin is really needed for plugin work so to don't break strings freeze we can only silently stop heaptrack analysis which is wrong and ugly. I think user should receive normal feedback.

So I suggest wait for 5.3 release and push this after. The patch will be available at 5.3.1 correcting release. Your opinion?

String freeze holds for the full time in the release branch. What usually is done in such a case where a bug fix really needs to break string freeze, is to ask translators for an exception. Usually a formality, but done at least to show respect to translators and their work.

Cmp. e.g. https://marc.info/?l=kde-i18n-doc&m=151092571015821&w=2 . Once two or more representatives of language teams have okayed (and no-one objected), exception is granted (that is documented somewhere, but could not find that quickly).

So if you see no way around, ask the translators now by emailing such a request to kde-i18n-doc@kde.org Will see to start reviewing in parallel then this WE.

+1, as on IRC.

This is for 5.3 branch.

Debian Stretch (current stable release) has Qt 5.7 and KF 5.28. Next stable release isn't until mid-2019.

But do they change things behind our back after this method has been entered?

Motivated by the recent code update of qmljs from QtCreator, which uses at least one feature that needs 5.8 (QT_CONFIG cpp macro), it might be time to consider an update of the min versions we require for Qt and KF.

the test would go into clang's test_duchain.cpp, create a method in there with the three TestFiles with the appropriate contents. Then parse them all and finally query the functions declarations and verify that the appropriate function definition is returned always.

Add more checks to the if-statements

Restructure if-statements in visitStaticMember.
Remove commented code from unit tests.

Feel free to push to 5.3 branch after fixing my remarks.

In D9344#346281, @rjvbb wrote:

The current patch I still cannot oversee (though also due to the existing code), so I would have to grab the -1 sign for now if on the jury.

Question is: are you, and should the refactoring be done before or after this change? I'm quite sure I won't have time for a serious overhaul the coming few weeks.

Abandoning for now given inactivity by the original author, for cleaning the to-review list. Still hoping one day someone/you will pick up this again.

Abandoning for now given inactivity by the original author, for cleaning the to-review list. Still hoping one day someone/you will pick up this again.

In D16356#347314, @mwolff wrote:

I assume you ran the tests (esp. for clang?) and it all keeps passing? if so, then +1

+2 if you could write a proper test for the case you explained in your commit message

How to proceed?

Ah, good point, indeed.

@kfunk plugins/externalscripts would need an uncrustify run as well (currently 2 space indentation on all files). I anticipated in this patch you will do this next :)

Any help on the Windows version is greatly appreciated, whenever you find the time for it. :)

In D15976#347700, @kfunk wrote:

@Petross404 If you want to reapply the registry-reading on top of that, feel free to do so. Sorry, but I dont think I'll have time to test that. :(

Superseded by https://phabricator.kde.org/D16123.

Hm, still not entirely convinced of all the added CMake code..., but at least the uses of the macros look better now.

Thanks!

Well, I guess adding line offsets is just the level of abstraction our code gen works at, anyways. Which is good enough. In my opinion, go for it ;)

Yes, seems that way.
I also remember seeing that on bugzilla, but can't find now.

Is the opening brace always on its own line, independent of the formatter selected?

I assume you ran the tests (esp. for clang?) and it all keeps passing? if so, then +1

• updated based on the suggestions from Milian Wolff

master sounds fine to me; after a bit of exposure we could backport to 5.3 branch, targeting 5.3.1.

I'd say master personally, but @brauch or @kfunk should decide that.

I do have push rights, I can land this tomorrow as I'll be travelling a bit today.
Which branch should this be landed against ?

thanks a lot! do you have commit rights, or should we push this for you?

Many tests will fail because of LSAN (leak sanitizer), which is enabled (at least here) by default with ASAN.

I'm inclined to say this looks innocent enough to go into 5.3 ...