Hm I agree that this is better than the status quo, and also think it's the right place to do it. But what about completion after typing "foo1" ? Then we still want to show completion. The only case when we don't want to do it, if the word at the cursor is a number. So can you change that code to do that maybe?

Remove insert API and use append everywhere.

I'm fine with his minus the change to return an empty path as that breaks the error message

++V

one more round of cleanups please

Name: Thomas Schöps, Email: tom dot schoeps aet gmail dot com
Thanks for committing this. I'll try using arcanist next time.

V3

Should I de-obfuscate and remove that lambda expression too while we're at it? Its body could be the body of clangBuiltinIncludePath() itself, AFAICT.

I'm afraid I don't know much about this either :-/
The fact that the unit tests are currently completely broken doesn't help. I did try them with and without this change and what I *can* say is that it's neither worse nor better.

btw, I just tested the v1 of this patch set just now and it worked like a charm (my kdev was build against clang 7.0.0, but I got updated to 7.0.1)

Updated as discussed in my previous comment. I've also followed flherne's suggestion to move the check for the reference file file into ClangHelpers::clangBuiltinIncludePath(). This does require some changes elsewhere because it can now fail (return an empty string).

In D17858#388948, @flherne wrote:

Perhaps the check found in plugins/clang/clangsupport.cpp:185 should be moved into this function

@pprkut can you look at this? I have not used PHP or its tooling for more than 10 years ...

Could you give me your full name + email for attribution in the commit?

I don't think that I have commit rights.

I think that looks fine. Didn't test though.

Why not.

Looks good to me. Can you push to 5.3 branch yourself?

5.3 only please. Someone else (or even you) can merge it into master afterwards.

In D18021#389167, @apol wrote:

Good catch!

Sorry for being late to the party. Checked this on FreeBSD - no regressions.

Good catch!

Looks good to me, let's get this in master?

+1 in general

On further consideration, I think I was wrong about this, sorry.

As a new feature I'd say it's just easier to get this in master.

Any objections for pushing this to 5.3 ?

Thanks for the input concerning the canonical paths. I have tested the plugin with different setups containing symbolic
links and haven't encountered any issues. Regardless, I have canonicalized the paths just to be safe.

• Use canonical paths

Yep, sorry, everything's alright.

Works great, good job!

• Fix some edge cases, cleanup and add more test cases

arrowd accepted this revision.
arrowd added a comment.

In D17278#386387, @dmensinger wrote:

I also don't know why the icons don't show up. I have oriented myself on the code from the CMake plugin, and I don't know what I am doing different/wrong. Maybe KDevelop has to be installed in a system directory (/usr) for this to work?

I also don't know why the icons don't show up. I have oriented myself on the code from the CMake plugin, and I don't know what I am doing different/wrong. Maybe KDevelop has to be installed in a system directory (/usr) for this to work?

Yep, that fixed the issue.

That is strange..
I bumped the version, so it should update it, I tried re-installing and it works for me.
Can you try deleting ~/.local/share/kxmlgui5/kdevclangsupport/kdevclangsupport.rc ?

It now compiles and unit test passes, however my own quick test revealed following problem.

This segfault is/was most likely caused by a missing Ninja builder plugin. I have added checks to prevent the segfault from happening.

• Make sure that the ninja buidler exists

Thanks for the feedback. I don't think I have commit rights, feel free to commit it for me.

Strange. I don't have this menu item even with your patch.

Before: https://i.imgur.com/GhGdOBV.png
After: https://i.imgur.com/ygDEnpw.png

• Remove dependency on clang 7.0.0 functions

Looks good to me.

Sorry for silly question, but where this change is reflected in KDevelop UI?

Tried this out on FreeBSD with KDevelop master.

Since 5.3.1 was released, can we get this in?

I have tried this out on master and it works for me. If no one objects in next few days, please push this.

Judging from current CI output, this has been fixed.

Another +1. @thomassc Do you have commit rights or you need someone else to commit?

The patch looks good overall +1

+1 makes sense to me.

As far as I can tell, the changed function is indeed responsible to determine whether to do *automatic* invocation of KDevelop's completion. Manual invocation by the user is still possible by pressing Ctrl+Space. The automatic word completion by KTextEditor, on the other hand, still works for numbers after this change. But it is far less intrusive since it only shows up after typing a few digits and if there are matching other numbers in the same file. So the probability that it offers useful completions is likely much higher, and I'd personally leave that one as it is.

Hmm, maybe the better way to achieve this effect would be to disable *automatic* invocation of completion after numbers? I already noticed that this is mostly undesirable, too. This is probably something you don't want to fix in the Clang plugin, but in KTextEditor instead ...

I made an extra release of Okteta today (0.25.5) which adds the -DBUILD_OKTETAKASTENLIBS=OFF (default:ON) option, so only the core libs can be built when wanted, like here :)

• Fixed typo

Thanks in advance for fixing this minor typo.

Users shouldn't change the dependencies and expect things to work without a rebuild.

I'm opposed to the concept of this patch.

Shouldn't these come from the project manager?

@mwolff ?

Shouldn't these come from the project manager?

Yes, I'm hoping that others will chime in here on this aspect too.

It's right. Seems we talk about the same thing. Summary:

• Plugin don't need in #ifdef to compile-time disabling, because...
• Plugin available and can be used at all platforms.
• Path to pkgconfig executable and environment vars are configurable from kdevelop settings menu.

Again, I strongly doubt that you can build kdevelop and all of its dependencies without having pkgconfig installed. Someone using kdevelop for development with libraries that provide .pc files will almost certainly have pkgconfig installed too. So really, the platform argument is moot IMHO. But if you really want to push it: as a Mac user I can guarantee that we (as in developers working on Mac) will have pkgconfig installed through one of a handful of package managers which we'll also be using to install the libraries we need for our development. IOW, pkgconfig *will* be installed in a more-or-less standard location (certainly considered standard on the local set-up) and you can bet it's on the path. Having used cygwin extensively in the past I am certain pkgconfig will be on the path in that universe too, and in the end that's all that counts (if you cannot configure the location of the executable).