Rene, I'm missing context - what are you replying to? This doesn't seem to be related to the preamble?

Rene, I'm missing context - what are you replying to? This doesn't seem to be related to the preamble?

Re: the time and resources spent by the background parser when you'd rather not:

Hmm, now I'm thinking: If we parse first without creating a preamble, we still have the Clang AST. Couldn't we create the preamble from the AST without reparsing and use that for subsequent parsing?

In D18551#605577, @mwolff wrote:

How bad would it be if we don't do that and only provide KTextEditor-completions before the first edit? I think it's pretty rare that code completion is immediately requested, typically one starts with a line break, or by removing code, before new code is written.

Very bad. I can still remember how bad it felt to use KDevelop before @brauch fixed some nasty issues that prevented us from using the PCH properly. It always felt like KDevelop was utterly broken since it took so long to give us completion answers. And I believe I also remember that quite a few of our users complained about it, and in turn praised the improvement after @brauch fixed it.

You're referring to D6905? That contains a lot of other things. I don't think that adding the flag was the core part of that change, and @brauch doesn't think so either:

You don't have control over where those pch files are written, right, other than through setting $TMPDIR?

In D18551#604847, @aaronpuchert wrote:

To keep this short I didn't reply to the parts I completely agree with.

In D18551#604249, @mwolff wrote:

Now I actually have /tmp as tmpfs, and so these 400 MB don't come for free. This is perfectly fine when I'm actually working on these files, but so far I have only opened KDevelop.

I see. The problem really is that the preamble files shouldn't be written to /tmp then - it would be perfectly fine to move them to a temp dir on the disk I think - it would still be faster than having to redo the work.

There are arguments for both in-memory and on-disk, it depends on how much memory there is. I have another more more generously sized machine where the files are perfectly fine in RAM.

I think /tmp is correct by the FHS and /var/tmp wouldn't be appropriate. I could easily put this on disk if I wanted to. I just wanted to make the argument that resource usage is a concern, and that there is some benefit in limiting ourselves to the preambles that we need.

To keep this short I didn't reply to the parts I completely agree with.

In D18551#544467, @aaronpuchert wrote:

I still think this is the right change, but if there is no consensus I will implement @mwolff's suggestion. That would certainly be an improvement, but it doesn't really solve my problem. I'm currently working on Clang and have around 6 files open, which is really not a lot. I haven't changed any of them. The result:

aaron:~/src/llvm-project> ls -l /tmp/preamble-*
-rw-r--r-- 1 aaron users 73139496 Okt  9 22:27 /tmp/preamble-0e19f4.pch
-rw-r--r-- 1 aaron users 59327864 Okt  9 22:23 /tmp/preamble-2a515c.pch
-rw-r--r-- 1 aaron users 61885536 Okt  9 22:23 /tmp/preamble-2baca3.pch
-rw-r--r-- 1 aaron users 76913424 Okt  9 22:23 /tmp/preamble-9853df.pch
-rw-r--r-- 1 aaron users 64254872 Okt  9 22:23 /tmp/preamble-cbb035.pch
-rw-r--r-- 1 aaron users 64829792 Okt  9 22:24 /tmp/preamble-dd9bf1.pch
aaron:~/src/llvm-project> du -s /tmp/ 2>/dev/null
391204  /tmp/

Now I actually have /tmp as tmpfs, and so these 400 MB don't come for free. This is perfectly fine when I'm actually working on these files, but so far I have only opened KDevelop.

Anyone who takes enough interest in kdev-pg-qt to care for review? :)

To be used then by https://invent.kde.org/kde/kdevelop/merge_requests/97

Plan is to do a 2.2.1 release directly after that (also needed to officially get the Qt 5.14 build-fix out), so this is available to kdevelop 5.5

Yes, it is

https://invent.kde.org/kde/kdevelop/merge_requests/93 is the replacement of this patch, right?
If so, please also close this review here as discarded :)

Hi @lbiaggi, welcome to contributing to KDevelop, thanks for your first(?) patch proposed here. I agree with your goal to make the code more modern :)

In D25494#592751, @bmwiedemann wrote:

This should work fine with a producer GNU-tar >= 1.28

We use the option only when SOURCE_DATE_EPOCH is set, and we assume that a recent enough tar is available then. Basically we say that if reproducible builds are desired, the proper tools should be available.

I would really like to have this in 5.5, so if there's no review by next weekend I'll merge as is.

No further need, thanks. I got the assist in your previous reply already I was looking for :)

So no urgent need for me to revisit the code?

This should work fine with a producer GNU-tar >= 1.28 , but what will be the consumer of the tar file?

In D25494#592508, @kossebau wrote:

No insight myself, but would trust you you know what you are doing here :) and it also matches what I just read on https://reproducible-builds.org/docs/archives/

No insight myself, but would trust you you know what you are doing here :) and it also matches what I just read on https://reproducible-builds.org/docs/archives/

Thanks for having had a look again, and confirming that I was not seeing things wrongly half-asleep still :) I am about to touch this code in some days anyway, and would twist things then as needed.

Difficult to wrap my head around this, so much later!

I just came across this logic (for reasons) and wonder if the logic does what you intended with the patch:
given the const auto canonicalPathToCheck = checkPathIsFile ? pathInfo.canonicalFilePath() : QStringLiteral("");, when checkPathIsFile is false, canonicalPathToCheck is an empty string.

add some tests

Looks good :)

fix: const are case sensitive
improve tests

Looks good :)

No problem.
You can use hugues@mitonneau.me
Thanks

Sorry this took so long! I have it ready to merge, just need to know the email you'd like me to use for the commit author

This new patch re-implement KDevelop::BasicRefactoring::applyChangesToDeclarations and KDevelop::BasicRefactoring::applyChanges in Php::Refactoring.

No, I don't have commit access.

I appreciate the effort, but I think this is the wrong approach. I really think the $ is part of the variable.

Hi! Sorry, I was just really busy and didn't have time to take a closer look.

Is there something wrong with the second diff ?

Thanks!

Looks good to me :)

Can someone else land the patch? I don’t think I have the required permissions.

Good enough as for me.

@starbuck Thanks. Tried a first link with https://store.kde.org/p/1334948 and adapted the KNSRC file of KDevelop, worked as wanted.
(well, once I added a workaround for some strange KNewStuff behaviour on archive extraction, cmp. 39a8a51d33 ;) )

I added the new category now here:
https://store.kde.org/browse/cat/606/order/latest/

Thanks for the reply, sounds good.

We certainly can add a new category = API Documentation QCH Files.
Linking files are possible via "Add url", simply pointing to a file as endpoint.
I'm not sure what needs to be done client-side, so Dan can likely help there.
I just need to know what the category name should be and when to add it.

@starbuck @leinir H. Please see "Question to store.kde.org managers" (but better to read first whole text to get the context). I would like to sort this out before end of year.

Move variadics from IndexedContainer to ArrayType

I agree that conceptually this should be moved to (or supplemented by) the actual language plugins, but also agree that this might be out of scope for this change.
As far as the PHP changes are concerned this is fine with me. However, I'd prefer those patterns to be unit tested and not just documented in comments.
Could you have a look at adding those tests?

Conceptually, this looks fine. However, like this we end up with two different descriptions of "Array of" types in the navigation popups.
While this shows integer arrays as int[], for variadics it will show array of (int). I think at the very least both should be displayed the same.

Rebased for the 5.4 branch. Still working perfectly for me, without noticeably slower reaction times on local filesystems.

Rebased for the 5.4 branch.

I'm not suggesting to not create a preamble at all, but to create it only when we **know** it is needed.

I've just stumbled into a case where I would need -stdlib=libc++, which this change would solve.

I still think this is the right change, but if there is no consensus I will implement @mwolff's suggestion. That would certainly be an improvement, but it doesn't really solve my problem. I'm currently working on Clang and have around 6 files open, which is really not a lot. I haven't changed any of them. The result:

e.g. look at https://doc.qt.io/archives/qt-4.8/qcolor.html#predefined-colors

hey! can you also show a screenshot of PHP or Qt documentation showing? these can contain arbitrary HTML and sometimes even contain colored text that expects to be shown on bright backgrounds which would break with this patch. At least that was the case years ago when I worked on this the last time.

Would it be possible and interesting to reuse code highlighting for the code quote ?

With the patch :