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

Document renumbering lines in macros #40955

Closed
wants to merge 7 commits into from
Closed

Conversation

bramtayl
Copy link
Contributor

So, this is something I've figured out on my own, so I'm not 100% sure if it's correct or not. If it's not correct, I'd greatly appreciate knowing what the correct way to handle this is.

@adds_more_lines_2 (macro with 1 method)
```

`__source__` will contain a line number pointing to the user's call site (see more information about `__source__` above).
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to link to the "macro invocation" section where the __source__ arg is defined rather than just saying "above", since that isn't too descriptive.

Copy link
Contributor

@imciner2 imciner2 left a comment

Choose a reason for hiding this comment

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

I found the term "definition expression" somewhat confusing in this because it actually isn't defined elsewhere in the docs and so was a new term. Instead I think it is simpler to talk about the macro itself in these cases.

bramtayl and others added 2 commits May 27, 2021 13:03
Co-authored-by: Ian McInerney <Ian.S.McInerney@ieee.org>
@bramtayl
Copy link
Contributor Author

Thanks for the suggestions! I was having trouble coming up with good terminology.

@imciner2
Copy link
Contributor

Yea, choosing terminology is always a difficult thing to do. I noticed one last thing that needs to be cleaned up (the parenthesis was missing), and after that it looks good to me (but you need to wait for someone with commit rights to approve this as well).

Co-authored-by: Ian McInerney <Ian.S.McInerney@ieee.org>
This is for the following two reasons:

- If an error occurs in code introduced in a macro, you likely want stack-traces to point to the user's call site, rather than the macro's internals.
- If a user calls a macro, you likely want coverage to consider the user's call site as covered, rather than the macro itself.
Copy link
Member

Choose a reason for hiding this comment

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

These argument seem inaccurate to me. The source should already be be getting represented as the place where the macro is being inlined at (as a fake "caller"). Injecting it again into the macro body seems like it could lead to even stranger frames as it tries to guess why this macro is changing back and forth between inlined and local. If the source location is missing in the caller (where we splice the macro to), that seems like just a bug.

There's already Base.remove_linenums! and associated improvements in PR form: #31335 and Meta.replace_sourceloc! from #22623.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the source location is missing in the caller (where we splice the macro to), that seems like just a bug.

Ok, if I understand you right, a bug like this is likely what I'm running into

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I close this and open a new issue?

@bramtayl bramtayl closed this Jun 1, 2021
@bramtayl
Copy link
Contributor Author

bramtayl commented Jun 1, 2021

Replaced by #41043

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