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 name_stable_ptr methods and GetName trait for AST objects #7254

Merged
merged 4 commits into from
Feb 11, 2025

Conversation

mkaput
Copy link
Member

@mkaput mkaput commented Feb 10, 2025

CairoLS needs to check whether an arbitrary TerminalIdentifier is a name of a given NamedLanguageElementId. This would be pretty tedious to implement downstream, while it is trivial to embed in the define_* macros we have here.

CairoLS needs to check whether an arbitrary `TerminalIdentifier` is a name of a given `NamedLanguageElementId`. This would be pretty tedious to implement downstream, while it is trivial to embed in the `define_*` macros we have here.
@mkaput mkaput requested a review from orizi February 10, 2025 14:43
@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Member Author

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @orizi)


a discussion (no related file):
@orizi if possible, I'd like to get this cherry-picked to 2.10. I need this to fix bugs in goto-definition implementation.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r1.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions


crates/cairo-lang-syntax/src/node/helpers.rs line 152 at r1 (raw file):

/// Provides methods to extract a _name_ of AST objects.
pub trait GetName {

Suggestion:

/// Provides methods to extract a _name_ of AST objects.
pub trait HasName {

crates/cairo-lang-defs/src/ids.rs line 58 at r1 (raw file):

pub trait NamedLanguageElementLongId {
    fn name(&self, db: &dyn DefsGroup) -> SmolStr;
    fn name_stable_ptr(&self, db: &dyn DefsGroup) -> TerminalIdentifierPtr;

will this work?

Suggestion:

    fn name_identifier(&self, db: &dyn DefsGroup) -> TerminalIdentifier;

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mkaput)

Copy link
Member Author

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @orizi)


crates/cairo-lang-defs/src/ids.rs line 58 at r1 (raw file):

Previously, orizi wrote…

will this work?

yes, it will. I stuck with stable ptrs because that's what I compare + I have originally been returning untyped stable ptrs because I wanted to reserve space for elements named after non-identifiers (like keywords) which kinda doesn't make sense in retrospective.

Done


crates/cairo-lang-syntax/src/node/helpers.rs line 152 at r1 (raw file):

/// Provides methods to extract a _name_ of AST objects.
pub trait GetName {

Done.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @mkaput)

@mkaput mkaput enabled auto-merge February 11, 2025 07:49
@mkaput mkaput added this pull request to the merge queue Feb 11, 2025
Merged via the queue into main with commit 20d1f79 Feb 11, 2025
48 checks passed
@mkaput mkaput deleted the mkaput/named-stable-ptr branch February 11, 2025 09:41
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