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

Add source code location to all named entities #302

Merged
merged 55 commits into from
May 16, 2024

Conversation

Schottkyc137
Copy link
Contributor

Closes #158

Now adds a source span to all named entity declarations.
Also adds unnamed statements and declarations (i.e., process statements) to the entity hierarchy (visible, for example, in the outline in VHDL).

Any feedback concerning the format is greatly appreciated.
For example, I'm unsure whether if statements and similar should be part of the outline or not.

@Schottkyc137 Schottkyc137 changed the title All source spans Add source code location to all named entities May 9, 2024
@pidgeon777
Copy link

For example, I'm unsure whether if statements and similar should be part of the outline or not.

Do you think it would be possible to let the user decide what should be included in the outline and what not? Maybe by using the LSP server settings during init, or the project config file?

Or, all of the basic VHDL if, for etc. tokens could be output, and then filtered out eventually client side.

I think it would be great to also report the when <value> of the case <name> is in the VHDL outline (useful to quickly jump to FSM states, for example).

@Schottkyc137
Copy link
Contributor Author

For example, I'm unsure whether if statements and similar should be part of the outline or not.

Do you think it would be possible to let the user decide what should be included in the outline and what not? Maybe by using the LSP server settings during init, or the project config file?

Or, all of the basic VHDL if, for etc. tokens could be output, and then filtered out eventually client side.

I'm not against a generic configuration. If you could provide a 'user perspective', i.e., how a user would like to configure the various options, I might include it if it isn't too much effort. That being said, such a feature isn't high on my priority list. Therefore I won't give any guarantees that this will actually be implemented. Probably this would belong to the initial server configuration as this is quite specific to the language server.

I think it would be great to also report the when <value> of the case <name> is in the VHDL outline (useful to quickly jump to FSM states, for example).

I see the benefit in such a feature, but I won't implement it now. At the moment, the outline only contains named object, i.e., entities, architectures, e.t.c. This also includes objects that could have a name in form of a label, such as if or case statements. when clauses do not fall into this category and would thus potentially require a more substantial refactor of the current design.

@Schottkyc137
Copy link
Contributor Author

@kraigher I'm sort of at a crossroad here. The source location of an AnyEnt is currently implemented using a TokenSpan object while the declaration position is implemented using a SrcPos object. I would like to make this more consistent so that either token information or source code location is given (i.e., either exchanging the declaration position with a single TokenId or exchanging the whole span with a single SrcPos location).
Having the SrcPos on an entity has the advantage of being easier, since no context is required to get the actual location, but may be less flexible since finding the original token span is more cumbersome. Having token information could be advantageous however, for example, for extracting documentation during hover. Do you have a preference how this should be implemented?

@kraigher
Copy link
Member

The reason to move in direction of TokenSpans was to enable better access to comments and lossless formatting. So it makes sense to keep going in this direction.

Using the context to get tokens is not so bad. Having a context on the side also means less information has to be stored in the data structure themselves which is more space efficient.

@Schottkyc137 Schottkyc137 marked this pull request as ready for review May 15, 2024 07:58
@Schottkyc137 Schottkyc137 merged commit 5d2038d into VHDL-LS:master May 16, 2024
10 checks passed
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.

[Bug] Format of textDocument/documentSymbol LSP response doesn't allegedly match the LSP specification
3 participants