I have been mega busy for the last while and intend to contribute more to this project when my situation stabilizes in like two weeks. Whenever you have time, I would definitely be interested to hear what sort of challenges you have come across through experimentation. An issue which I have suspected from the onset is actually determining all that needs to be protected by DUChain's lock and those which don't since KDevelop source makes use of these locks so liberally. I'm wondering if in your experimentation there are open questions about the DUChain locking scope.

Work-around pushed as 8a18a08cf17c85000c7082d5192855430a519bab , seems to pass build also with neon ci and the gcc 7.4.0 there.

Seems the introduced QStringLiteral as default argument here in the template method executeCompletionTest() makes ubuntu bionic's GNU 7.4.0 C++ compiler screw up. At least there is some timely coincidence that the KDevelop builds on neon CI started to fail after this commit, with this error:

05:00:37 [100%] Building CXX object plugins/clang/tests/CMakeFiles/test_codecompletion.dir/test_codecompletion.cpp.o
05:00:37 plugins/clang/tests/CMakeFiles/test_codecompletion.dir/build.make:62: recipe for target 'plugins/clang/tests/CMakeFiles/test_codecompletion.dir/test_codecompletion.cpp.o' failed
05:00:37 make[4]: Leaving directory '/workspace/build/obj-x86_64-linux-gnu'
05:00:37 /tmp/ccrWtnBp.s: Assembler messages:
05:00:37 /tmp/ccrWtnBp.s:65174: Error: symbol `_ZZNK12_GLOBAL__N_1UlvE_clEvE15qstring_literal' is already defined
05:00:37 /tmp/ccrWtnBp.s:68144: Error: symbol `_ZZNK12_GLOBAL__N_1UlvE_clEvE15qstring_literal' is already defined
05:00:37 make[4]: *** [plugins/clang/tests/CMakeFiles/test_codecompletion.dir/test_codecompletion.cpp.o] Error 1
05:00:37 CMakeFiles/Makefile2:18121: recipe for target 'plugins/clang/tests/CMakeFiles/test_codecompletion.dir/all' failed
05:00:37 make[3]: *** [plugins/clang/tests/CMakeFiles/test_codecompletion.dir/all] Error 2

from https://build.neon.kde.org/job/bionic_unstable_extras_kdevelop_bin_amd64/220/console

In D22559#498486, @aaronpuchert wrote:

In D22559#498026, @kfunk wrote:

I do not think bumping versions "just because " has any value.

I kind of agree here. Unless we have features that we know are only available with a certain version, we might needlessly restrict how people can build this. They might assume we have a good reason for choosing that specific version and that older versions can't possibly work. When I package stuff for openSUSE, I would never try decreasing minimum versions of dependencies, because I assume that these are actually the minimum. On the other hand, I regularly stumble across software that doesn't specify minimum versions for dependencies, but assumes it's "reasonably recent".

In D22182#495823, @mswan wrote:

Also, in spite of what I said earlier, I think the explicit DUChain locking should be done and there eventually should be some work done to determine which objects will only interact with one specific DUChain per instance of that object, i.e. an object that should reference a specific DUChain, e.g. ClangSupport. We could make the first pass at this involve replacing all of those DUChainReadLocker lock; lines with DUChainReaderLocker lock(DUChain::lock());, [...]

Overall I think this is a good idea, although we need to figure out some details. Could you either remove commented out flags or add a comment why we have them (commented out)?

I don't know what would be required to support multiple DUChains, but it seems plausible that they can be locked separately. In this case it would help to have the explicit argument, I guess. It could even be inevitable, when there is no single global DUChain anymore.

In D22559#498026, @kfunk wrote:

I do not think bumping versions "just because " has any value.

Can we have a test for this?

There is another condition here: `if (lastShownMenu)`. So if the QMenu object instance got deleted

Add rtti flags since they affect defines.

Prefer not duplicating code.

Just do the simple thing. Copied the one line of code due to
the awkwardness of preprocessor directives inside template lists.

As the person who raised the Debian Stretch issue last time, I don't think it's worth caring about now that Buster is released.

Sorry for my conservative thinking again: Debian Buster (10) is still /very/ fresh; it was just released a couple days ago. The former "oldest" supported distro was Debian Stretch (9) with Qt 5.7 and KF 5.28. That'll stick around for some time.

(Using phabricator to make sure people are triggered for this central change)

In D22424#497415, @rjvbb wrote:

I'll activate the debug trace on change of my lastShownMenu though

I'm doing this (in my patch from D16882 which still applies after reverting D22424):

+        if (menu != lastShownMenu.data()) {
+            if (lastShownMenu) {
+                qCWarning(SHELL) << "Added items to new contextmenu" << menu;
+            }
+            lastShownMenu = menu;
+        }

under the assumption that this will print a warning (enabled in qtlogging.ini) when lastShownMenu will be updated but not the 1st time a ctx menu is opened. Is that assumption wrong, because I'm not seeing any warnings after disabling the CTags menu (and restarting KDevelop for good measure).

I'll activate the debug trace on change of my lastShownMenu though

In any case there isn't much else you can do; stepping through the code with a debugger is near impossible (the menu about-to-be-opened will already have grabbed mouse and keyboard focus so you'd need to display remotely).

In D22424#497361, @rjvbb wrote:

How do you think I debugged the situation? I did the same thing in Kate too.

Please add a line

  qDebug() << "Showing context menu" << menu;

to KDevelop::TextDocument::populateContextMenu.

In D22424#497316, @rjvbb wrote:

Since we're dealing with "please read/see again": idem for my comment :P which only states that a single QMenu instance is being (re)used. I've never seen it change

>>   > FWIW I got to look at the KTE implementation of the context menu mechanism that is used here. It indeed uses and reuses a single QMenu instance (there's even a comment in the code about that).
>>   
>>   Please give links into the code, as I am lost what you exactly refer to here.
> 
> see the source of `KTextEditor::ViewPrivate::contextMenu()`. The ctx menu is in fact managed by KXMLGUI.

The context menu is queried every time from KXMLGUI that method is called.  So  if KXMLGUI internally decides to recreate the context menu, you get another object the next time. Please see again the description of this very bug.

In D22424#497271, @rjvbb wrote:

The other option would have been to release 5.3.3 without a fix for ctags plugin users, rendering it unusable for people relying on packaged kdevelop.

Erm, aren't you forgetting about my patch which could have been applied to the (EOL!) 5.3 branch? That does the job just as well and would have left time to find a fix for #409858 in time for the next release.

The other option would have been to release 5.3.3 without a fix for ctags plugin users, rendering it unusable for people relying on packaged kdevelop.

In D22424#497204, @rjvbb wrote:

Friedrich W. H. Kossebau wrote on 20190717::00:12:44 re: "D22424: TextDocument: remove actions from contextmenu on hide already"

This revision was not accepted when it landed; it landed in state "Needs Review".

Hmm, since when is this acceptable?

Friedrich W. H. Kossebau wrote on 20190717::00:12:44 re: "D22424: TextDocument: remove actions from contextmenu on hide already"

In D16882#494867, @rjvbb wrote:

I could try your solution, of course, but what annoys me is that it comes months after I worked on mine.

In D22424#496112, @kossebau wrote:

In D22424#494980, @rjvbb wrote:

However, the patch has a side-effect: I'm getting messages like this each time I switch or open documents:

kdevplatform.shell: Broken text-document:  QUrl("file:///opt/local/var/tmp/kdevelop-tmp-505-%7Bf793a513-f14e-47b9-8448-06ca72900c04%7D/kdevelop_Q27955.patch")

This example is for a file that indeed doesn't exist, but that I also do not have open (so WTH?!) but I also get it for files that open perfectly.

I fail to see an immediate reason for this, so I'm back to running my own patch.

Interesting. Not seen here, but giving this some stress testing now. The kdevelop_xxxxx.patch is coming from the diff review system creating text document objects for displaying the diffs, but I also see no direct relation ship to this patch here. Will poke around a bit,

update to first review feedback

Thanks for first review & testing.

In D22456#496025, @apol wrote:

LGTM can you land it?

LGTM can you land it?

Okay, that all makes sense to me. Also, in spite of what I said earlier, I think the explicit DUChain locking should be done and there eventually should be some work done to determine which objects will only interact with one specific DUChain per instance of that object, i.e. an object that should reference a specific DUChain, e.g. ClangSupport. We could make the first pass at this involve replacing all of those DUChainReadLocker lock; lines with DUChainReaderLocker lock(DUChain::lock());, but ideally we would eventually come to some understanding about which objects are instantiated within a specific DUChain context such that multiple instances can coexist with entirely independent DUChain interactions. The eventual use case for this is something like T11223 which at this point has so very many unknowns that it would be difficult to start tailoring changes like proposed here to that end. At this stage with the desired changes loosely described above, I am inclined to think that there needs to be some explicitly stated semantics of DUChain::lock(), i.e. it locks all DUChain instances. If we eventually start adding support for multiple DUChain instances we would at least not have to worry about any of these existing locks being overly permissive given the suggested default semantics. I don't know if anything I have just said changes anything about how we would go about applying Clang Thread Safety annotations to this project, but I suspect that it does. Thoughts?

• Added missing header for column with session name

LGTM otherwise

I've tested this in the 5.3 branch now, which required applying hunk #3 manually to texteditor.cpp .

I could try your solution, of course, but what annoys me is that it comes months after I worked on mine. I currently have too many other things going on in my life to dive in and figure out what on earth was going on again. I do think I outlined it well enough above in the initial description and/or exchange; I should find a moment this weekend to sit down and re-read it with a fresh mind.
It would help if you had a specific critique on my solution other than "it doesn't use this or that signal" (or, what I kind of sense, "it comes from you"). No disrespect intended, but your description in D22424 isn't that easy to read either (it felt like reading German, for some inexplicable reason ;) ).

Just a few remarks on the comments that should make them easier to understand (a priori comments should illustrate code and not require lots of different code the understand their meaning ;))

Reasons why currently I favour this over D16882:

• slightly less complex, as it does more what one expects, adding & removing actions on show & hide
• no global static

In D16882#494702, @rjvbb wrote:

Please check the earlier discussion; IIRC there is a reliability problem with that signal, and I did try reverting to its use before coming up with the current solution.

Please check the earlier discussion; IIRC there is a reliability problem with that signal, and I did try reverting to its use before coming up with the current solution.

While trying to understand the problem, I found that perhaps we could still revert to using the aboutToHide signal, with proper boundaroes.
So alternative solution proposal up as D22424

Even in contexts where you are specifying the struct itself, it is still matching against the typedef.

In D21589#494068, @Petross404 wrote:

In D21589#492408, @sredman wrote:

Actually, there is a bit of a problem... The version of the file currently on master is bugged :(

See: https://phabricator.kde.org/D17908#492404

Any progress with the MS tool?

Hm, seeing all this hardcoded code for optional plugins, this calls out for someone to look into making this something pulled in from the language plugins instead :) Not your fault, just mentioning the obvious. So should be fine to just add here.

In D21589#492408, @sredman wrote:

Actually, there is a bit of a problem... The version of the file currently on master is bugged :(

See: https://phabricator.kde.org/D17908#492404

In D17908#492404, @sredman wrote:

@Petross404 are you still around? Do you have any documentation references for how Microsoft expects the registry to look with regard to which key should contain the Visual Studio installation path? (I don't know if such documentation even exists...)

Remove debugging printf.

Moved to whitelist.

Change to workingDirectory, add comment.

In D22182#492087, @mswan wrote:

The workaround being that we eliminate the try-lock behaviour in our locker constructors and move that timeout behaviour elsewhere?

Exactly.

• Fix action boolean.

Actually, a pleasant surprise for today is that this whole mess of registry keys and environment variables is mostly unnecessary as of VS 2017, since Microsoft provides a tool which does all the locating work for us: https://github.com/microsoft/vswhere

Actually, there is a bit of a problem... The version of the file currently on master is bugged :(

Unfortunately, I have a problem with using the batch file from this diff on my system.