Parser support for explicit storage locations #15463
Conversation
matheusaaguiar
force-pushed
the
storageLocationsParserSupport
branch
from
42172bc
to
a632251
Compare
| @@ -591,6 +593,7 @@ class ContractDefinition: public Declaration, public StructurallyDocumented, pub | |||
| std::vector<ASTPointer<ASTNode>> m_subNodes; | |||
| ContractKind m_contractKind; | |||
| bool m_abstract{false}; | |||
| ASTPointer<Expression> m_storageBaseLocationExpression; | |||
There was a problem hiding this comment.
Should we create a dedicated AST node for it?
| ); | ||
|
|
||
| advance(); | ||
| return parseExpression(); |
There was a problem hiding this comment.
I guess it should be something more specific, because expression will accept anything. Or is it fine?
There was a problem hiding this comment.
Yes, I also believe that we should have something more specific. If I recall correctly, it must be a constant expression that can be evaluate at compile-time. So maybe we should have a new parse function that is restricted to TokenTraits::isArithmeticOp, Literals and the erc7201 helper only.
There was a problem hiding this comment.
I am assuming that we will check those kind of things later (constant, compile-time eval, etc) at type checking phase.
I am worried about parsing something weird (like a function declaration or a nested contract declaration), but I think I should look more closely to the behaviour of parseExpression to get a better feeling for that.
matheusaaguiar
force-pushed
the
storageLocationsParserSupport
branch
from
fef6ff4
to
26bb5e5
Compare
matheusaaguiar
force-pushed
the
storageLocationsParserSupport
branch
from
26bb5e5
to
bf640be
Compare
| solAssert(m_scanner->currentLiteral() == "layout"); | ||
| expectToken(Token::Identifier); | ||
| if ( | ||
| m_scanner->currentToken() != Token::Identifier || |
There was a problem hiding this comment.
Identifier check is redundant here - it'll throw a fatal parsing error in expectToken(...) if it's not an identifier.
commented previously here.
@nikola-matic , I am not sure if you were suggesting that we use expectToken instead of if to check the identifier with literal at.
I used if here because we also need to check for the literal value at.
Or were you referring to expectToken in line 376? (in which case, it actually is consuming the identifier with literal layout).
There was a problem hiding this comment.
Yup, you are correct. You could do the same as above (check that literal is at and then expectToken(Identifier), but then you'd just get the generic error Expected identifier but got..., and since we're not introducing new tokens (at, layout), it makes sense for error to be specifically tailored like it is here.
One suggestion I could make is to have the error follow the general expectToken() convention, I.e. Expected 'at' but got '..', since I assume we're gonna introduce layout and at tokens (as well as transient) for the next breaking release.
There was a problem hiding this comment.
One suggestion I could make is to have the error follow the general
expectToken()convention
Yeah, I will make this change.
|
This pull request is stale because it has been open for 14 days with no activity. |
github-actions
bot
added
the
stale
matheusaaguiar
removed
the
stale
matheusaaguiar
force-pushed
the
storageLocationsParserSupport
branch
from
bf640be
to
666b499
Compare
First step for #597.