Implement DIP1009 correctly (#303)#304
Conversation
Codecov Report
@@ Coverage Diff @@
## master #304 +/- ##
=========================================
- Coverage 83.62% 83.5% -0.12%
=========================================
Files 7 7
Lines 4817 4784 -33
=========================================
- Hits 4028 3995 -33
Misses 789 789
Continue to review full report at Codecov.
|
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Sorry but the official grammar is much more complex. Reference document is here : https://github.com/dlang/DIPs/blob/master/DIPs/accepted/DIP1009.md#grammar. Your implementation goes in the right direction but more new nodes are required.
|
I had based this on the way |
LaurentTreguier
force-pushed
the
issue-303
branch
from
7683e5f to
7a7be30
Compare
|
I've tried to follow the changes of DIP1009 more closely. |
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Nice work. Maybe just the in/ouTokenLocation to remove
| { | ||
| mixin (visitIfNotNull!(assertArguments)); | ||
| } | ||
| /** */ size_t inTokenLocation; |
There was a problem hiding this comment.
I don't think this is required. IIRC you wanted this for DFMT but with the official grammar FunctionContract gives the information if it's a expression style contract (its InOutStatement is null) or a statement style contract (its InOutContractExpression) is null.
There was a problem hiding this comment.
inTokenLocation and outTokenLocation were already used for detecting classic contract statements; the problem with them was simply that they weren't filled for expression-based contracts, so DFMT didn't consider them as contracts.
But inTokenLocation and outTokenLocation are used to distinguish contracts from other usages of in and out, and not to distinguish expression-based ones from statement ones, so I think it's probably better to leave them there
There was a problem hiding this comment.
ok then. since my comment i just remembered too that DFMT is based on the tokens and only very partially on the AST.
| { | ||
| mixin (visitIfNotNull!(parameter, assertArguments)); | ||
| } | ||
| /** */ size_t outTokenLocation; |
There was a problem hiding this comment.
same as comment related to InContractExpression
src/dparse/parser.d
Outdated
| } | ||
| ownArray(node.functionContracts, contracts); | ||
|
|
||
| if (current.type == tok!"do" |
src/dparse/parser.d
Outdated
| ownArray(node.functionContracts, contracts); | ||
|
|
||
| if (current.type == tok!"do" | ||
| || (current.type == tok!"identifier" && current.text == "body")) |
There was a problem hiding this comment.
if (moreTokens && ... to prevent any RangeError.
src/dparse/parser.d
Outdated
| trace("Found register"); | ||
| mixin (nullCheck!`(node.register = parseRegister())`); | ||
| if (current.type == tok!":") | ||
| if (currentIs(tok!":")) |
There was a problem hiding this comment.
Please only address to the locations indicated. This usage is unrealted to DIP1009 and is fixed in #302
LaurentTreguier
force-pushed
the
issue-303
branch
from
f8edca4 to
6d828b4
Compare
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Thanks, I see no problem anymore. But will keep it opened for a few days before merging if someone else wants to review.
ghost
added
the
2 participants
This PR adds the following changes from the API perspective:
InContractExpressionandOutContractExpressionclasses to represent expression-based contractsinContractExpressionsandoutContractExpressionsproperties forFunctionBodyto store themassertionandmessageproperties forInvariantin the case of an expression-based invariantThese are obviously breaking changes since they change the way
FunctionBodyandInvariantstore expression-based contracts.