Compensate depth for compound blocks inside HashIf content#134
Compensate depth for compound blocks inside HashIf content#134jvsqzj wants to merge 3 commits intointel:mainfrom
Conversation
src/analysis/parsing/statement.rs
Outdated
|
|
||
| if let Content::Some(statement::StatementContent::Compound(_)) | ||
| = self.truebranch.content.as_ref() { | ||
| aux.depth -= 1; |
There was a problem hiding this comment.
Will this work correctly when, say, true-branch is a compound but else-branch is not?
There was a problem hiding this comment.
We did some inspection of this scenario with the dmlc compiler and it really does not allow to have one liners without braces in #if or #else blocks.
I think this syntax requires that the braces are always present, as the compiler fails when trying to compile such scenarios (and likewise the DLS AST also throws a syntax error in those cases) so I would not worry about having blocks which are not compound (they will always be, apparently)
There was a problem hiding this comment.
#if/else cannot have single-statements on object level, but on statement level it is allowed.
See statement::HashIfContent (statement level) as compared to structure::HashIfContent (object level)
Sidenote: these probably should not have been called the exact same thing (even if in separate modules), I might rename one or both to more clearly distinguish the types.
jvsqzj
force-pushed
the
handle-compound-content-in-hashif
branch
from
06714ae to
cb229d5
Compare
| if self.should_increment_depth() { | ||
| aux.depth += 1; | ||
| aux.depth = aux.depth.wrapping_add(1); | ||
| } |
There was a problem hiding this comment.
@JonatanWaern we'd like to hear your comments on this solution.
We are using this u32 value of 0xFFFF_FFFF to signal the child code block of a top-level #if/#else block it should actually use a zero-level indentation (ie remain at the top level).
We did not want to change the type of aux.depth to i32 as there will never be a negative indentation, so IMO it would add far more possibilities of undefined behavior to allow for negative values here than to just interpret the biggest possible value as a boundary.
The wrapping_add takes care of taking the depth to the corresponding value accordingly; either 0 for top level, or n+1 for any other nested #if/#else
There was a problem hiding this comment.
I dislike in-type magic values in rust on principle, and AFAIK it's best practice to avoid. In particular setting it to a magic max value, and relying on a wrapping add seems hacky to me.
Instead, it may be that indentation is sufficiently complicated that we should introduce a dedicated structure/enum to support it? (which could support incrementation and hard-set overrides) I have some ideas for this, so I may try my hand at it later (at the moment I need to get the builds to work again)
There was a problem hiding this comment.
I have prepared a draft for what such a handler could look like in #185, you could rebase on it to see if it fits for what you need
2 participants
No description provided.