Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

scip-ctags: add kinds support for C #58082

Merged
merged 2 commits into from
Nov 6, 2023
Merged

scip-ctags: add kinds support for C #58082

merged 2 commits into from
Nov 6, 2023

Conversation

mrnugget
Copy link
Contributor

@mrnugget mrnugget commented Nov 2, 2023

This is part of https://github.com/sourcegraph/sourcegraph/issues/58024 and adds scip-ctags support for C.

Test plan

  • Unit tests

@cla-bot cla-bot bot added the cla-signed label Nov 2, 2023
@mrnugget mrnugget changed the title Mrn+pjl/scip ctags c scip-ctags: add kinds support for C Nov 2, 2023
@github-actions github-actions bot added the team/source Tickets under the purview of Source - the one Source to graph it all label Nov 2, 2023
Comment on lines +20 to +22
static uint sweet_sweet_numbers[5] = {23, 420, 69, 42, 7};
// ^^^^^^^^^^^^^^^^^^^ definition(Variable) scip-ctags sweet_sweet_numbers.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@varungandhi-src
Copy link
Contributor

I've mentioned some missing test cases inline. Some more test cases to add:

  • K&R style declarations. example
  • Union declarations
  • Make sure that we're not emitting symbols for type declarations in nested contexts (such as inside another type definition, inside a function body etc, inside a parameter list) (as those aren't accessible from a global scope). E.g. struct z { struct w { int x; int y; } b; } -- we shouldn't emit a global descriptor for w here.

@mrnugget
Copy link
Contributor Author

mrnugget commented Nov 3, 2023

Thanks @varungandhi-src! union popped into my head earlier, but not the other things. I'm on it.

@mrnugget
Copy link
Contributor Author

mrnugget commented Nov 3, 2023

Another thing: pointers to pointers!

struct connection **my_ptr_ptr;

@mrnugget
Copy link
Contributor Author

mrnugget commented Nov 6, 2023

Okay, in the name of simplicity I simplified a lot of things:

  • Added "support" for most of the constructs mentioned above
  • Dropped "Constants" as a separate kind, because it made the queries essentially unmaintainable (due to recursive nature of declarations and the modifier being top-level).
  • Queries are much simpler now, at the cost of fidelty: we don't tag StaticVariable, we don't tag Constant -- but we correctly tag a const int ****ptrptrptrptr;
  • Dropped the whole thing of fields in anonymous unions shadowing global variables, because that would require modifying the Rust code.
  • Added tests for scip and tags.

I think that's in the spirit of what we're after here, @jtibshirani, right? Let me know if we aim for more correctness, then I'll take another stab.

Otherwise, I think we can always follow-up and tweak some things.

@jtibshirani
Copy link
Member

@mrnugget the simplifications look great and seem like the right trade-off. Our intended use cases for symbol kinds don't require a high level of precision (it's just ranking and showing the kinds for symbol search). I'll review again shortly.

@mrnugget
Copy link
Contributor Author

mrnugget commented Nov 6, 2023

Update: talked with @tjdevries a bit and he also confirmed that getting const/static working reliably and fixing the anonymous union thing would require more than just a couple of hours of query writing -- would need to change the Rust code. Happy to do that, of course, but wanted to check, @jtibshirani, whether that's even wanted or not.

Copy link
Member

@jtibshirani jtibshirani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! I see all the important kinds to detect.

@mrnugget mrnugget merged commit baa50d7 into main Nov 6, 2023
9 checks passed
@mrnugget mrnugget deleted the mrn+pjl/scip-ctags-c branch November 6, 2023 16:44
vovakulikov pushed a commit that referenced this pull request Dec 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed team/source Tickets under the purview of Source - the one Source to graph it all
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants