FunctionDefinition: only look for (new/other) function definition if we don't have one
ClosedPublic

Authored by buschinski on Oct 21 2018, 3:48 PM.

Details

Summary

FunctionDefinition::definition always looks for FunctionDefinition based on the DeclarationId, but we don't need to do that if we already have a FunctionDefinition.
This could cause problems if we have multiple (same) definitions.

Test Plan
  • create a small cmake (or whatever) project with 3 targets
    • util1.c / util2.c / util3.c (same content):
#include <stdio.h>
#include <stdlib.h>

void configure()
{
    printf("do stuff in %s\n", __FILE__);
}

int main(int argc, char *argv[])
{
    configure();
    puts("Hello, UTIL!");
    exit(EXIT_SUCCESS);
}
  • CMakeLists.txt:
add_executable(util1 util1.c)
add_executable(util2 util2.c)
add_executable(util3 util3.c)
  • open util1.c
  • open the outline and select "configure"
  • EXPECTED: it should open the configure in util1.c (or the currently opened file)
  • repeat with util2 and util3
  • ACTUAL: only one of them works, the other two point to the wrong file.

Diff Detail

Repository
R32 KDevelop
Lint
Lint Skipped
Unit
Unit Tests Skipped
buschinski created this revision.Oct 21 2018, 3:48 PM
Restricted Application added a project: KDevelop. · View Herald TranscriptOct 21 2018, 3:48 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald Transcript
buschinski requested review of this revision.Oct 21 2018, 3:48 PM
mwolff added a subscriber: mwolff.Oct 21 2018, 6:07 PM

I'd say this new code should go into FunctionDefinition::definition instead, no? I.e. check there whether decl is already a definition and if so return that directly?

otherwise +1 for the idea, I wonder why this hasn't been done/spotted before ;-)

mwolff requested changes to this revision.Oct 21 2018, 6:08 PM
This revision now requires changes to proceed.Oct 21 2018, 6:08 PM

looking at the code, it really is missing there. You'll need a const cast but that's OK

buschinski updated this revision to Diff 44082.Oct 22 2018, 5:41 PM
buschinski retitled this revision from quickopen: only look for (new/other) function definition if we don't have one to FunctionDefinition: only look for (new/other) function definition if we don't have one.
buschinski edited the summary of this revision. (Show Details)
  • updated based on the suggestions from Milian Wolff

thx for the review :)

mwolff accepted this revision.Oct 22 2018, 6:22 PM

I assume you ran the tests (esp. for clang?) and it all keeps passing? if so, then +1

+2 if you could write a proper test for the case you explained in your commit message

This revision is now accepted and ready to land.Oct 22 2018, 6:22 PM

I assume you ran the tests (esp. for clang?) and it all keeps passing? if so, then +1

+2 if you could write a proper test for the case you explained in your commit message

Yes, it keeps passing.

But I really struggle in writing a proper test for this. I need big help here.
For starters I am even struggling with the correct test location, is it test_quickopen.cpp?

As different tests have different helpers and stuff.

I am guessing a (class) "TestProject" could help me but I could not find an easy on how to interact with the quickopen UI element and trigger it to open/activate a document?
So basically I am really lost here :\

the test would go into clang's test_duchain.cpp, create a method in there with the three TestFiles with the appropriate contents. Then parse them all and finally query the functions declarations and verify that the appropriate function definition is returned always.

buschinski updated this revision to Diff 44466.Oct 29 2018, 9:40 PM
  • Initial test, works with the patch, fails without :)
mwolff accepted this revision.Oct 31 2018, 8:25 AM

please cleanup the test slightly and then push it directly (no need for another round of review)

thanks!

plugins/clang/tests/test_duchain.cpp
2118 ↗(On Diff #44466)

wrap it in a lambda and call it three times with the file as input argument

Updated. Thanks! I will push it :)

buschinski marked an inline comment as done.Oct 31 2018, 12:36 PM
buschinski closed this revision.Oct 31 2018, 12:42 PM