To put it bluntly: you should have asked that 3 months ago ;)
I really can't remember if I had a specific reason or if this is just the result of a search/replace.

In D17908#401595, @mwolff wrote:

you can pick whatever you prefer for the reviews for now.

do you have commit rights? if so, please feel free to push this directly, otherwise someone from us can take care of that for you

you can pick whatever you prefer for the reviews for now.

Aaron, don't your arguments apply to parsing an entire project on startup as well? I can easily imagine that opening and parsing entire projects of the size you mention must have a non-negligible cost regardless of all optimisation and caching that is currently implemented
There is a setting for this feature. A similar switch for the preamble thing will probably be difficult to label comprehensively but maybe a coupling can be made with the full-project-parse switch?

Obsoleted by https://invent.kde.org/kde/kdevelop/merge_requests/5

Preambles are there to speed up repeated reparsing. Most translation units won't be reparsed in a typical session. Since we pay for every preamble we create, I don't think we should create them unless we've some indication that a file needs to be parsed again. It's pretty safe to assume that a file needs to be reparsed again if we parse it for the second time. There is probably no better indicator, which is why this is the default.

In D17760#401252, @mwolff wrote:

do we have custom patches in our libastyle?

Upload the minimum diff.

we use CXTranslationUnit_CreatePreambleOnFirstParse to get code completion results fast. otherwise the first code completion request would create the preamble, which felt much worse

On my test file, time spent in clang_codeCompleteAt goes down from ~700ms

do we have custom patches in our libastyle? I hope not, but it's been too long for me to remember. If not, then I guess we should get rid of our copy of lbiastyle and just disable kdevastyle if that dependency wasn't found - it's pretty optional anyways.

yes, push to 5.3 and we'll merge it to master

lgtm now, sorry for the delay - can you push that yourself? if not, please tell us your email address and name to associate with the commit

it was measured back then:

Replying to a remark by Milian on D17289 where this idea came up:

Hello all, No I do not have dev account, you can associate this patch to
wcancino@gmail.com

ping?

we use CXTranslationUnit_CreatePreambleOnFirstParse to get code completion results fast. otherwise the first code completion request would create the preamble, which felt much worse

please paste compiler errors instead of showing screenshots

sorry for the long delay. personally, I like what I'm seeing in the video, could you cleanup the patch a little please? and does ensureVisible help maybe?

sorry for the long delay, it sounds like a good idea but the diff is super hard to review since it contains lots of unrelated whitespace changes

great initiative, but could you check if this could be done in a more generic way potentially? if not then I'm all for getting this in as-is otherwise

This new patch (hopefully) addresses the test flakiness that I observed before:
https://phabricator.kde.org/D18567

In case someone thinks that creating the pch file when you open a file causes a noticeable lag during the opening process: if so this would also be the case when configuring KDevelop not to parse the entire project on import. I use that mode all the time and never noticed any such lag.

Wouldn't that be an opportunity to add the override specifier? The commit message could state "Fix function signature of ... to override".

lgtm now, note that you now need to push to the new gitlab remote at git@invent.kde.org:kde/kdevelop.git

lgtm, thanks. note that you now need to push to the new gitlab remote (git@invent.kde.org:kde/kdevelop.git) cf. https://invent.kde.org/kde/kdevelop for future reviews

since noone else is using this plugin, I'd say let's push this! @wcancino do you have a developer account? if not, then we can commit this for you. Please then give us your email address to associate with the commit then.

@thomassc: A separate context could help, yes. But if you find alternative ways to handle it, like proposed here, then not using a separate context could potentially work out too. But it feels a bit hackish to handle the template args specially everywhere instead of putting them into a proper context.

It'd be interesting to have an estimate of how much faster this makes that initial parse.

finally got it working, man that old code was broken :D I wrote most of that a really long time ago, and it shows!

In D17289#400446, @rjvbb wrote:

I've tried to unlink certain files after they were created but reverted that again when I started getting crashes in the parser. Not that reverting helped against that...

Yes, I believe that's not going to work. Clang doesn't keep the preamble files open, but expects them to stay.

sadly my initial attempt doesn't work either ;-) I'm looking at this some more now and will let you know once I've found an alternative approach

There are mechanisms to ensure cleanup in any event by removing an open file on POSIX compatibles (close will then

I've had a look at the Clang code. They actually go to some lengths to make sure all temporary files are deleted on a clean exit — they register all open temporary files and clean them up in an exit-time destructor. (See TemporaryFiles in lib/Frontend/PrecompiledPreamble.cpp.) However, this won't work in the case of a crash. They have some mechanisms for cleanup in the case of crashes, but I suspect that this works by setting a signal handler and is thus only available in the stand-alone executable. There are mechanisms to ensure cleanup in any event by removing an open file on POSIX compatibles (close will then automatically delete it), or FILE_FLAG_DELETE_ON_CLOSE on Windows. But that doesn't work for Clang, because they work with paths and don't pass file descriptors around.

No problem, I could work with my patch :)

sorry for the long delay, I just tested this and I can reproduce the crash. But just like Aleix, I'm opposed to the suggested way of fixing it. The patch increases coupling without solving the underlying problem. To me, it looks like the list job simply must not store raw pointers, as that's inherently unsafe when we think about changes being done during the queued signal emission... I think to fix this properly, we need to refactor the listjob, and I have an idea on how to do that (operate on indexed strings instead of items, only lookup item when listjob has finished). I'll try this out now and get back to you afterwards.

This version saves the original TMPDIR value, and adds a wrapper around QProcessEnvironment::systemEnvironment() that restores that original value. To keep changes to a minimum I've added that wrapper to the Core class itself.

lgtm, thanks

I think it's fine to commit this as-is

Updated as discussed.

Where or how can I find what to put in the JSON comments?

Do you think we should check for the version of Plasma to make a distinction between DrKonqi versions?
I think not, the Debug feature is not (yet) very popular, but I would like another opinion.

I was referring to @mwolff's comment D17289#395163.

To some extent that problem is going to disappear as soon as concepts are more widely used. GCC ≥ 6 already supports them and I expect Clang to follow suit — there is already a prototype.

In D17289#399019, @rjvbb wrote:

If you don't use systemd, for example because you're not on Linux, there are certainly other tools for doing the same thing.

How does that systemd thing clean tmp dirs at runtime, IOW, how can it know it's safe to clean up a given file if the application that created it doesn't do something explicit to guarantee cleanup?

so, I believe we need to change some things here...

please just push it to master

please add tests for the features you add - it's probably enough to add one or two files to tests/files/ and use the JSON comments to verify everything is parsed properly?

If you don't use systemd, for example because you're not on Linux, there are certainly other tools for doing the same thing.

How does that systemd thing clean tmp dirs at runtime, IOW, how can it know it's safe to clean up a given file if the application that created it doesn't do something explicit to guarantee cleanup?

In D17289#398555, @rjvbb wrote:

In D17289#395491, @aaronpuchert wrote:

Temporary files being left behind after a crash is not unusual. This is why systemd has systemd-tmpfiles --clean.

Not really relevant outside of the systemd realm, right? :)

I completely forgot about this one, for some reason the patch was moved out of my active set (possibly because of lineheight calculation issues).

In D17289#395491, @aaronpuchert wrote:

I haven't pushed anything since KDE svn days but my account should be set up I think. One way to find out. Which branches? master and 5.3?
Or just 5.3 and you're merging to master once in a while?

without digging into the patch myself (I'll have to see whether I can find time for this), could we solve more of the open TODOs by reporting problems for *all tail* child diagnostics that point at the current file? And then reference the "parent" from them somehow? I.e. I believe currently, we have something like this (bare with me, it was quite some time since I worked on this):

unrelated - but potential future work: does keyboard navigation work across tooltips? I.e. 'ALT + ARROW'?

I moved the code into helper functions and added an initial version of the tests. The tests for having the function and the call in the same file, and for having a chain of template functions in the same file include QEXPECT_FAIL(). In principle it would be easy to generate additional problems for these cases, but then a single actual problem would be represented by multiple problems in the same file. Maybe it would be cleaner to make a single problem have multiple ranges.

Make helper functions and add tests, as requested by Milian.

I just noticed this old bug: https://bugs.kde.org/show_bug.cgi?id=368460
Should the template parameters be put into a separate context?

Already fixed.

Yes, the behavior of this patch is as described by Milian.

This is how it looks on my system:

Cosmetic improvement to combined problems and decl tooltip

Can you push the change or do you need us to do it for you?

• Fix testcase

In D17289#370477, @rjvbb wrote:

Actually, issues with clang temp files is why I started to think about this kind of change but ultimately I realised it didn't seem such a bad idea at all to put all KDevelop-related temporary files in a dedicated location. I often clean out a bunch of KDevelop's own temp files that were left behind, e.g. after a crash. I just didn't mention them because they're negligible in size (and I purge before their numbers really start to grow).