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

Fix locations in presense of nested trivia #1035

Closed
wants to merge 1 commit into from

Conversation

povik
Copy link
Contributor

@povik povik commented Jun 18, 2024

@MikePopoloski I want to check with you to: confirm there's a bug, and see about a preferred way of addressing it.

Trivia t = token.trivia()[0];
CHECK(t.kind == TriviaKind::Directive);
REQUIRE(t.syntax()->kind == SyntaxKind::DefineDirective);
CHECK(t.getExplicitLocation()->offset() == 0);
Copy link
Contributor Author

@povik povik Jun 18, 2024

Choose a reason for hiding this comment

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

This check fails, the returned location points to the start of the directive and doesn't count the nested trivia that precede it.

@MikePopoloski
Copy link
Owner

Hmm, I'm not sure if this is actually a bug or not. It's currently only used when printing the syntax tree in a way that tries to get back the original source text, in which case I think it doesn't actually matter. Are you trying to use it in a way where this matters?

@povik
Copy link
Contributor Author

povik commented Jun 21, 2024

Yes, I am reconstructing the trivia locations in a way suggested by this comment

    // The vast majority of trivia lives alongside the token it's attached to, so if
    // you want to know its source location just walk backward from the parent location.
    // For certain cases though (the preprocessor inserted tokens) the trivia gets glued
    // together from different locations. In that case hasFullLocation will be true and
    // the union will point at a FullLocation structure.
    bool hasFullLocation = false;

With the location pointing to the start of the underlying directive syntax and not to the source text to which the trivia corresponds, I am not sure how to go about reconstructing the locations reliably. I guess you would need to run the syntax tree printer on trivia.syntax()->getFirstToken().trivia(), and then use the length of the returned text to offset the location (or something effectively the same without constructing the printed text).

Ideally, for the downstream use of slang I am working on, nested trivia wouldn't be possible and the trivia would be moved to the upmost token where it can live: that is, instead of there being any trivia in some_trivia.syntax()->getFirstToken().trivia(), the trivia would be moved up to belong to the token to which some_trivia corresponds. @MikePopoloski do you think such a change can be made consistently? Would you welcome it?

@MikePopoloski
Copy link
Owner

I think that would be fine; the existing code already assumes that if there is a trivia with an explicit location that all preceding trivia is implicitly relative to it, so I think it would just work. I'm not sure how annoying it would be to find all the places where this can occur, but it would at least be contained to the Preprocessor class. Feel free to attempt and let me know how it goes.

@povik
Copy link
Contributor Author

povik commented Jun 21, 2024

OK, thanks

@MikePopoloski
Copy link
Owner

Closing this for now; feel free to reopen if you want to put up a fix for the test.

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.

None yet

2 participants