-
Notifications
You must be signed in to change notification settings - Fork 6.1k
Fix shift in Yul AST native src locations due to code snippets in debug info #15457
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -90,13 +90,15 @@ class YulStack: public langutil::CharStreamProvider | |
| Language _language, | ||
| solidity::frontend::OptimiserSettings _optimiserSettings, | ||
| langutil::DebugInfoSelection const& _debugInfoSelection, | ||
| langutil::CharStreamProvider const* _soliditySourceProvider = nullptr, | ||
| std::shared_ptr<ObjectOptimizer> _objectOptimizer = nullptr | ||
| ): | ||
| m_language(_language), | ||
| m_evmVersion(_evmVersion), | ||
| m_eofVersion(_eofVersion), | ||
| m_optimiserSettings(std::move(_optimiserSettings)), | ||
| m_debugInfoSelection(_debugInfoSelection), | ||
| m_soliditySourceProvider(_soliditySourceProvider), | ||
| m_errorReporter(m_errors), | ||
| m_objectOptimizer(_objectOptimizer ? std::move(_objectOptimizer) : std::make_shared<ObjectOptimizer>()) | ||
| {} | ||
|
|
@@ -137,9 +139,7 @@ class YulStack: public langutil::CharStreamProvider | |
| bool hasErrors() const { return m_errorReporter.hasErrors(); } | ||
|
|
||
| /// Pretty-print the input after having parsed it. | ||
| std::string print( | ||
| langutil::CharStreamProvider const* _soliditySourceProvider = nullptr | ||
| ) const; | ||
| std::string print() const; | ||
| Json astJson() const; | ||
|
|
||
| // return the JSON representation of the YuL CFG (experimental) | ||
|
|
@@ -172,6 +172,11 @@ class YulStack: public langutil::CharStreamProvider | |
| solidity::frontend::OptimiserSettings m_optimiserSettings; | ||
| langutil::DebugInfoSelection m_debugInfoSelection{}; | ||
|
|
||
| /// Provider of the Solidity sources that the Yul code was generated from. | ||
| /// Necessary when code snippets are requested as a part of debug info. When null, code snippets are omitted. | ||
| /// NOTE: Not owned by YulStack, the user must ensure that it is not destroyed before the stack is. | ||
| langutil::CharStreamProvider const* m_soliditySourceProvider{}; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that the best solution is to just store a pointer to the source provider (i.e.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think that the lifetime is a big worry in this case, so fully agree! |
||
|
|
||
| std::unique_ptr<langutil::CharStream> m_charStream; | ||
|
|
||
| State m_stackState = Empty; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we want to have a changelog entry for this one. Technically, we never made Yul AST output non-experimental. On the other hand, the feature has been there for a while and it's still nice to have a record of what changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is good to have some documentation, even if the feature is experimental. Could be worth clarifying in the changelog that it is indeed an experimental feature, though?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think it is ok to have a changelog entry here, since we also included it when the feature was added in https://github.com/ethereum/solidity/pull/14177/files#diff-c275fec9c453c2a42515bc5ab47e30fa4130ff1426aef2f6e000a9a34e122cb8R8
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but that was actually an oversight back then :) I was meant to be experimental. And also we do have entries for introduction of experimental features, just not necessarily for updates to them. But given that it was not marked as experimental, perhaps this one is fine not being marked as such either...