One last round of ASTScope cleanups #34232
Conversation
slavapestov
force-pushed
the
misc-astscope-cleanups
branch
from
b38f7f4 to
09fa595
Compare
davidungar
left a comment
davidungar
left a comment
There was a problem hiding this comment.
Great stuff! Worth landing ASAP. Take my comments as fodder for any subsequent PRs you want to do.
lib/AST/DeclContext.cpp
Outdated
There was a problem hiding this comment.
Great to see the assertion here! A couple of thoughts:
-
A lambda inside the function vs a separate function:
In favor of the lambda is that it is only called here. In favor of a separate function is thataddMemberSilentlywould be a bit more readable without the lambda in the middle. Also the data flow intocheckSourceRangegets clearer when forced to pass in data explicitly. (Obviously I lean towards the latter.) -
Does it make sense to generalize the order checking to any Decl? The details would be different for brace statements, but it would be nice to reify the commonality of needing source ranges to be ordered.
BTW, great to check the invariant right at the insertion point!
There was a problem hiding this comment.
Also, I think some "why" comments could help here. Why check the ranges? Why skip over vars, enums, etc.?
There was a problem hiding this comment.
I'll add some more comments, thanks.
I'll also see about adding a similar assertion to SourceFile::addTopLevelDecl() and BraceStmt::setBody() (and hopefully share code), but I'll do that in a separate PR.
slavapestov
force-pushed
the
misc-astscope-cleanups
branch
from
09fa595 to
4b0d39c
Compare
|
@swift-ci Please smoke test |
|
@swift-ci Please test source compatibility |
|
@swift-ci Please smoke test Linux |
|
@swift-ci Please clean smoke test Linux |
|
@swift-ci Please smoke test Linux |
|
@swift-ci Please test source compatibility |
2 participants
Builds upon #34226.