Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Symbol grammar clarifications #255

Merged
merged 9 commits into from
Jun 27, 2024

Conversation

kritzcreek
Copy link
Contributor

@kritzcreek kritzcreek commented Jun 25, 2024

  • Forbids empty or 'local' scheme (this removes ambiguities from the grammar)
  • Unifies "any UTF-8" wording
  • Specifies that identifiers that can be encoded as simple identifiers, have to be encoded as simple identifiers

Unfortunately I can not get the "build process" to work on my local machine.

Test plan

Added checks to the lint command for checking canonical identifiers and that parsing succeeds

@varungandhi-src
Copy link
Contributor

We could add checks to scip lint

Sure, that'd be great.

@kritzcreek
Copy link
Contributor Author

kritzcreek commented Jun 25, 2024

Sure, that'd be great.

Allright, I think that should work. PTAL

Copy link
Contributor

@varungandhi-src varungandhi-src left a comment

Choose a reason for hiding this comment

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

Added some minor non-blocking comments

bindings/go/scip/symbol.go Outdated Show resolved Hide resolved
cmd/scip/lint.go Outdated
}
sym, err := scip.ParseSymbol(symbol)
if err != nil {
// TODO: This should be linted (but it makes all the tests fail)
Copy link
Contributor

Choose a reason for hiding this comment

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

Writing the long symbol names makes writing tests tedious, ideally we'd have a config object to disable some checks in tests. Maybe you can use a global variable here? It'd be off by default, and only one test would set it when checking for this code path. The other tests wouldn't need to worry about producing grammatically correct symbol names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about introducing a constructor function for the tests? Something like sym("a") that expands to . . . . a#?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can move that logic to makeIndex or parseSymbolInfo? In hindsight, I'm not sure if adding a DSL to specify relationships was a good idea, but I didn't want to have large chains of functions/methods when writing test cases. 🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I tried... but the DSL was a bit too much too handle so I went with the global as you suggested :D

@kritzcreek
Copy link
Contributor Author

Noticed one more difference between the spec and scip-clang while writing some benchmarks. scip-clang will generate descriptors like my_func()., while the spec does not make the disambiguator in the parenthesis optional.

Spec issue, or scip-clang issue?

@varungandhi-src
Copy link
Contributor

varungandhi-src commented Jun 26, 2024

Noticed one more difference between the spec and scip-clang while writing some benchmarks. scip-clang will generate descriptors like my_func()., while the spec does not make the disambiguator in the parenthesis optional.

Hmm, I don't see why the disambiguator should be forced to be non-empty -- the rationale for having the disambiguator is that different disambiguator values can correspond to different method overloads (there is no further sub-structure inside it). An empty string is just another string, which is different from other non-empty strings. Unless someone has a compelling reason otherwise, I think we should allow the disambiguator to be empty.

@kritzcreek
Copy link
Contributor Author

Unless someone has a compelling reason otherwise, I think we should allow the disambiguator to be empty.

@olafurpg @keynmol Thoughts?

@olafurpg
Copy link
Member

+1 to allowing empty disambiguators

@kritzcreek kritzcreek force-pushed the christoph/symbol-grammar-clarifications branch from dcd0d06 to 49acfd1 Compare June 27, 2024 05:56
@kritzcreek kritzcreek merged commit e7242d5 into main Jun 27, 2024
5 checks passed
@kritzcreek kritzcreek deleted the christoph/symbol-grammar-clarifications branch June 27, 2024 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants