Should I land this on master or 5.3 ?

Created new test case for whitespace mismatch.

Updated to use QChar overload as suggested.

I actually changed the last test to also test for this. Should I split it to a new one ?

can you also add a test for this new behavior with white space?

looking at the code, it really is missing there. You'll need a const cast but that's OK

I've changed the implementation to accomodate for whitespace, by comparing
a whitespace-stripped version of the signature and the leading text.

In D16326#346194, @amhndu wrote:

Added tests in testCompleteFunction, which already has some implements-completion
tests. Should I move them to a different test, something like testImplementExecute
in a different patch or this one ?

Completion tests are also broken, crashing after executing a single test case
because of a delayed connection, triggering the slot only after the document is closed.
As a workaround, I've used

diff --git a/plugins/clang/tests/codecompletiontestbase.cpp b/plugins/clang/tests/codecompletiontestbase.cpp
index 5b351d0d27..282f7da36e 100644
--- a/plugins/clang/tests/codecompletiontestbase.cpp
+++ b/plugins/clang/tests/codecompletiontestbase.cpp
@@ -70,6 +70,7 @@ void CodeCompletionTestBase::initTestCase()
     m_projectController = new TestProjectController(core);
     core->setProjectController(m_projectController);
     ICore::self()->documentController()->closeAllDocuments();
+    QObject::disconnect(ICore::self()->documentController(), nullptr, nullptr, nullptr);
 
     ClangSettingsManager::self()->m_enableTesting = true;
 }

Which still has a crash, but only at the very end after executing everything.

I'd say this new code should go into FunctionDefinition::definition instead, no? I.e. check there whether decl is already a definition and if so return that directly?

very good idea

lgtm in general, three small nitpicks and I'll have a look at the broken unit test now too.

Can go to 5.3 in my opinion. Thank you!

Sorry, forgot to push it. Should it go to 5.3 or master?

Thx for the review :)
I will update the patch (and post it here again).

Well, make test works for me, and if this fixes the issue for you, I'm fine with that.

One more thing: could establishing the project file name not simply be done by a method in the ProjectController class rather than by a dedicated class? Because, in how many different functions would you split that logic?

Dominik is more well versed in cstyle.js ;=)

Arguably that still stands, but it's outside my area of expertise to comment on it.

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.

The signal handling and processing gets a +1 from me, but someone else should review the overall code structure.

Rereading the patch and the related code once more, my personal opinion now is: this logic has been confusing before and only gets more confusing with the proposed patch.

updated as requested.

Added tests in testCompleteFunction, which already has some implements-completion
tests. Should I move them to a different test, something like testImplementExecute
in a different patch or this one ?

Would it be possible to get a test for this?

Drops the potentially unsafe qDebug output in the signal handler, makes handlingSignal an std::atomic_bool (since we can use locks here anyway) and adds copious comments.

Still working on this and waiting on more background info from the Qt Interest ML.

You can't use std::atomic_flag, because it's an `int`.

In D16218#345150, @rjvbb wrote:

So is it enough to change to std::atomic_flag or should we use some sort of critical section with an actual lock? Because for handlingSignal that's possible as it is outside of the actual signal handler.

Any idea how we could test the race-condition scenario? I've been trying by re-raising the signal from just before and just inside the if(handlingSignal) loop and until now it has always been behaving as expected (= the if is executed only once).

This is determined by hardware support. With relevant architectures I meant architectures on which Qt/KDE applications can run, like x86, PPC, ARM. > These all have lock-free `std::atomic<bool>`, as far as I know.

I guess you can also use `std::atomic_flag`, which is guaranteed to be lock-free, and should offer enough functionality for our use case.

...

Not in the middle of an assignment, but between the read in `if (!handlingSignal)` and the write `handlingSignal = 1` it can be interrupted. That's not atomic.

Go for it. I don't have an opinion on whether to use version 1 or 2 from an API point of view, both dont look entirely clean to me.

In D16218#344435, @rjvbb wrote:

At least std::atomic<bool> is lock-free on all relevant architectures. (I'm aware that the standard doesn't guarantee it, probably because of some embedded architectures.

But is it always lock-free on the relevant architectures? Or rather:

• what determines if the type is lock-free
• what are "the relevant architectures"?

In D16203#344806, @brauch wrote:

I'm not a huge fan of tuples either, for the reason you mentioned; I usually just create a two-element struct instead. In this case I think it's fairly obvious what the elements do from the types, though; the QPair<int, int> is worse ;)

I'm not a huge fan of tuples either, for the reason you mentioned; I usually just create a two-element struct instead. In this case I think it's fairly obvious what the elements do from the types, though; the QPair<int, int> is worse ;)
Up to you how you do this, I just wanted to point out that it is done differently in two similar places.

Use QPair also with ContextBrowserPlugin::navigationWidgetForPosition

In D16203#344770, @brauch wrote:

I think the change conceptually makes sense, and I am subconsciously aware of the bug it is trying to fix. If you say it works for you, it should be fine.

I think the change conceptually makes sense, and I am subconsciously aware of the bug it is trying to fix. If you say it works for you, it should be fine.

At least std::atomic<bool> is lock-free on all relevant architectures. (I'm aware that the standard doesn't guarantee it, probably because of some embedded architectures.

Updated as suggested

Where in the code would you want a documentation of the intent of this change?
The best place might be a in the handbook or the project import wizard GUI ... but we're in string freeze, no?

You're right, my remarks apply to the existing code already.

In D9344#339889, @rjvbb wrote:

HtH...

• Dropped the semaphore-based alternative as it would only be less resource-intensive when manhandling pthreads
• moved almost everything back out of the CorePrivate class, keeping only the slot required for QSocketNotifier operation.
• dropped the SIGHUP shortcut, to be resubmitted in the future

I would recommend to just not think about the Windows requirement, if you cannot test it anyway.

In D16218#343384, @rjvbb wrote:

If there is no solution which does not require two different code paths for handling this problem, I'm against merging this, non-conformant old behaviour or not, sorry.

So to repeat myself: if signal handling should work on MS Windows (and safely so) then the QSocketNotifier approach is off the table, in principle. I cannot (and do not want to) be judge of whether MS Windows should be supported here.

The semaphore-based implementation should be more direct as it doesn't go over the Qt event loop and signal/slot mechanism. That makes me inclined to prefer it.

In D16218#343380, @rjvbb wrote:

• Why does this patch look so much more verbose than the solution proposed on http://doc.qt.io/qt-5/unix-signals.html?

Probably because it's a patch against existing code, which needs changes due to the fact that I thought I'd had to avoid static globals.

If there is no solution which does not require two different code paths for handling this problem, I'm against merging this, non-conformant old behaviour or not, sorry.

• Why does this patch look so much more verbose than the solution proposed on http://doc.qt.io/qt-5/unix-signals.html?

If there is no solution which does not require two different code paths for handling this problem, I'm against merging this, non-conformant old behaviour or not, sorry.

Nice find! Will cherry-pick to 5.3 branch.

Sorry, but this is yet another unmergeable WIP patch of yours. Just a quick review, I'm not diving into your code (which is difficult to parse, esp. the changes in CorePrivate::initialize...) right now.

In D16218#343348, @rjvbb wrote:

Oh, misunderstanding! I simply don't have a full KF5 build environment on FreeBSD yet. I'm running TrueOS in a VM on a system that isn't exactly powerful, so I'm waiting until "Project Trident" is released with a full KF5 desktop option.

In D16218#343340, @arrowd wrote:

Adds the missing #endif

In D16218#343326, @rjvbb wrote:

Re: the testing I did on FreeBSD: that's with a dedicated demonstrator; I cannot currently build KDevelop5 there.

Re: the testing I did on FreeBSD: that's with a dedicated demonstrator; I cannot currently build KDevelop5 there.

The simple example

namespace A
{
    void foo();
}

@Petross404 Hmm, true. Let me push my patch anyway, for now.

Reduce to only two public macros, using TYPE argument to define details

I hate tο be a naysayer here, but shouldn't we not assume that VS2017 is installed in C:\Program Files (x86) so that we don't hardcode it's path? I remember my self installing VS once in another drive, because C:\ was tiny and full.

I also find this reasonably simple to read. The "type your compiler arch" thing is extremely 80s of course, but the whole concept is a hack :/

This batch file seems very clean and simple, I like it. Unfortunately I can't help reviewing this patch as I no longer have access to Windows machines and I don't a dual-boot system :(

In D16032#341315, @apol wrote:

In D16032#340778, @kossebau wrote:

In D16032#340668, @apol wrote:

Considering we're doing our own macro, we can be also more succint. I don't see why we can't have declare_plugin_qt_logging_category(targetname)

You can use target_sources() instead of adding it to the variable.

You mentioned something about target in an earlier comment. But I still have no idea what you mean here and what should be generated how, could you please give an explicit example, to help aligning my brain with yours? :)

Does that make sense to you?

<snip sample>

In D16032#340778, @kossebau wrote:

In D16032#340668, @apol wrote:

Considering we're doing our own macro, we can be also more succint. I don't see why we can't have declare_plugin_qt_logging_category(targetname)

You can use target_sources() instead of adding it to the variable.

You mentioned something about target in an earlier comment. But I still have no idea what you mean here and what should be generated how, could you please give an explicit example, to help aligning my brain with yours? :)

Check whether base is defined at another place