AST: make isNotAfter a strict weak ordering
#27805
Conversation
|
CC: @davidungar |
|
@swift-ci please test |
lib/AST/ASTScopeCreation.cpp
Outdated
| @@ -617,7 +617,7 @@ class ScopeCreator final { | |||
| #endif | |||
| ASTScopeAssert(startOrder * endOrder != -1, | |||
| "Start order contradicts end order"); | |||
| return startOrder + endOrder < 1; | |||
| return startOrder < 0 && endOrder < 0; | |||
There was a problem hiding this comment.
This is a semantics change, though. Exact matching ranges, for example, should be considered "not after".
(I'm also a little confused by this whole idea. One range nested entirely inside another will trip the assert.)
There was a problem hiding this comment.
Yeah, it was very confusing. I think that <= should maintain the strict weak ordering, so that seems like a better approach. I'll change that.
There was a problem hiding this comment.
Thank you, @compnerd for working on fixing my oversight. Thank you, @jrose-apple for flagging me. I believe that this change is incorrect, but so is the existing code I wrote.
The ordering here is used for sorting, which is used in ASTScopeImpl::findChildContaining. That function compares a location against the End of the ranges only. What is needed is a factoring: a function that does the comparison and is used by both the sort routing and the findChildContaining routine. Otherwise, we'll have hiding places for bugs. The factored routine should just use the End locations. (I originally chose that because @DougGregor 's prototype used that.)
In addition to factoring (and keeping the assertion), I wonder if the assertion should be turned into a test that crashes the compiler, even in production runs. The rationale would be that a miscompile will result from incorrect lookups when the ranges overlap. What do you both think?
There was a problem hiding this comment.
I'm not really certain how to refactor this. The findChildContaining simply queries the SourceManager, and the comparator doesn't really generalise very much. Did you have something in mind? I'd like to get this resolved ASAP, this is breaking the Windows build.
`std::stable_sort` requires that the comparator provide a strict weak ordering, which we did not previously. This was caught by MSVCPRT.
compnerd
force-pushed
the
strictly-strengthening-weakness
branch
from
7e2955a to
485bb5d
Compare
|
@swift-ci please test |
|
@davidungar - ping? This is blocking debug builds on Windows :-( |
| @@ -617,7 +617,7 @@ class ScopeCreator final { | |||
| #endif | |||
| ASTScopeAssert(startOrder * endOrder != -1, | |||
| "Start order contradicts end order"); | |||
| return startOrder + endOrder < 1; | |||
| return startOrder <= 0 && endOrder <= 0; | |||
There was a problem hiding this comment.
After looking at this again, I'm suspecting that the right answer is an assertion that endOrder != 0 and a simple test for endOrder < 0. But I haven't tried it yet. I'll run some tests this weekend. (I would also keep the existing assertion.)
There was a problem hiding this comment.
Thanks for fixing this, I'll test it out locally.
There was a problem hiding this comment.
Happy to do so. Thank you for your patience.
std::stable_sortrequires that the comparator provide a strict weakordering, which we did not previously. This was caught by MSVCPRT.
Replace this paragraph with a description of your changes and rationale. Provide links to external references/discussions if appropriate.
Resolves SR-NNNN.