great, thanks for the help - please land the other change then

@jkorous ping? can you chime in on either of the two patches? I'm fine with either, and D82629 comes with a test too. So maybe let's just go with that one? If so, could you integrate that one please?

this relates to https://reviews.llvm.org/D82740 too, but comes with a test which is good. personally I would place the check directly within Visit to make it more generically prevent a crash on invalid input, but that's probably a subjective design decision.

Also see https://reviews.llvm.org/D82629 which I believe is also a fix for this crash. But I didn't use a VLA in my code, but maybe the parse error just triggered something similar like that.

@jkorous how would you use a debugger (would be GDB for me) to find the source - I would have to use RR or something like that to see why and where the invalid node is added, no?

I'm not sure how to write a unit test for this, but I ran into a reproducible crash with a complex C++ file which got fixed by this patch

ping?

ping?

with this patch I could just use the global lit and don't need to add the build dir to my PATH. But, thinking about it, I can also symlink llvm-lit into a custom folder in my PATH and see if that works. If you don't want to merge this, I'll abandon it, please tell me if you would prefer it like that.

I'm using lit 0.10.0 installed via pip 20.0.2 using python 3.8.2

nope, without the symlink I get:

fixup commit message

thanks!

Do you have commit rights? Can you push that for me?

@nik odd, it works for me, I just switched to master (previously I used the 8x release branch) and applied this patch and build it all. Then I run:

looks good, but this needs a test, could you add one please?

I'm pretty sure this is not ABI compatible. You change the size of CXCompletionResult, so when anyone is iterating over the array in CXCompletionResults it will break. If you don't want to, I can try to find some time to take this over again ;-)

still looks good to me. can someone else please review and commit this?

Can this be abandoned now that the issue Erik has mentioned got fixed?

I don't think this upholds the ABI guarantee, but maybe I'm wrong? Can someone else chime in please? @klimek ?

lgtm, but could we get some more tests rather than only this one for templates?

superseded by https://reviews.llvm.org/D37650

In D13599#856273, @klimek wrote:

In D13599#539899, @milianw wrote:

Long time no see - sorry for the delay. I want to pick this up again. Would you say I should add the tests as separate tests (i.e. something like -code-completion-cursors-at), or should I rather output the cursor in addition when requesting -code-completion-at, and then extend all existing unit tests with the cursor information?

As it's a new function, I'd add a separate test.

lgtm in general, but this patch is missing a unit test

agreed, lgtm but someone else must accept this upstream

Long time no see - sorry for the delay. I want to pick this up again. Would you say I should add the tests as separate tests (i.e. something like -code-completion-cursors-at), or should I rather output the cursor in addition when requesting -code-completion-at, and then extend all existing unit tests with the cursor information?

ping, any update on this?

+1 from my side, but someone else has to approve

+1 from my side, but someone else has to approve.

Still good from my side. @klimek, @rsmith: Could you please review this as well?

lgtm - I should have done that directly. Sadly, I didn't receive my commit rights either yet ;-)

@akyrtzi raises a very valid point - I did not think about that. KDevelop uses the clang-c API and does fuzzy matching on top of the results, e.g. for camel-case matching. But, we currently always request code completion at a word start boundary so nothing would change for us. That said, I see how this patch could be seen as a breaking behavior change, and thus should probably only get enabled by an explicit option - if at all.

I'm not yet acquainted enough with the code at hand, but I wonder whether I'm understanding the code correctly:

Thanks Manuel!

attended klimek's review comments

Ping? This patch would be very helpful for us to have in KDevelop. Can anyone give us a review please? I'm willing to amend it as needed to get this issue resolved.

closing then, since this has been landed

lgtm, but someone else should approve

lgtm, but someone else should approve

From my POV this is still fine. Manuel, Sergey - could you have a look at this please and push it upstream if you agree with me?

From my POV, this looks good.

+1 from my side, but someone else has to approve

Looks good to me!

Adding tests wouldn't hurt though, quite the contrary. Especially if it wasn't tested before. Otherwise, this looks good to me.

This looks good to me, but it's missing a unit test. Take a look at http://reviews.llvm.org/D13388 for how to do that in principle.

Yep, looks good to me as well - thanks!

any suggestion as to where put unit tests for this?

Is there still an error reported for the invalid condition?

Ping? Can someone please submit this upstream?