Fix up the deep equality demangler tests. #78812
Conversation
|
cc @DmT021 |
|
@swift-ci please smoke test |
| @@ -251,6 +251,7 @@ class Node { | |||
| } | |||
|
|
|||
| static bool deepEquals(const Node *lhs, const Node *rhs) { | |||
| assert(lhs && rhs && "Nodes being compared may not be null"); | |||
There was a problem hiding this comment.
If both pointers can't be null we don't need if ((!lhs && rhs) || (lhs && !rhs)). Or we can require at least one of them to be non-null, then the assert should be lhs || rhs.
There was a problem hiding this comment.
Also, if both of them may not be null it might be a good idea to take const references instead
There was a problem hiding this comment.
Good catch about the now-redundant comparison; removed it.
I think we want to keep the types as pointers, because NodePointer (type alias of Node *) is the "currency" type used by the demangler; there are no public APIs that consume a Node &, so requiring the caller to dereference it in just this case would be a bit unusual.
These tests were broken. The `Demangler::demangleSymbol` function operates subtly differently than what `swift-demangle` does under the hood (which powers the `mangling.txt` lit test), which calls calls `Demangler::demangleSymbolAsNode` instead. That difference caused many valid partial manglings to fail and return `nullptr`. Unfortunately this was never caught, because `isDeepEqualTo` immediately passes `this` to a static method (so there was no actual null pointer dereference occurring), and then the two `nullptr`s were compared for equality, erroneously returning true. I only happened to catch this when running the test in an environment with more strict pointer enforcement. I've updated the tests to: * Separate the valid manglings from invalid manglings so that we can test both outcomes. * Assert inside `deepEquals` that neither pointer is `nullptr`. * Assert that the symbol, when demangled and printed as a string, matches the expected demangling (which was present in the `.def` file but never used). The latter point ensures that all behavior is being verified, making these tests equivalent to those in `manglings.txt` with the added nondeterminism check that was in the original PR.
allevato
force-pushed
the
fix-demangling-tests
branch
from
5b851e6 to
d4ee054
Compare
|
@swift-ci please smoke test |
|
@swift-ci please smoke test Linux platform |
These tests were broken. The
Demangler::demangleSymbolfunction operates subtly differently than whatswift-demangledoes under the hood (which powers themangling.txtlit test), which callsDemangler::demangleSymbolAsNodeinstead. That difference caused many valid partial manglings to fail and returnnullptr.Unfortunately this was never caught, because
isDeepEqualToimmediately passesthisto a static method (so there was no actual null pointer dereference occurring), and then the twonullptrs were compared for equality, erroneously returning true. I only happened to catch this when running the test in an environment with more strict pointer enforcement.I've updated the tests to:
deepEqualsthat neither pointer isnullptr..deffile but never used).The latter point ensures that all behavior is being verified, making these tests equivalent to those in
manglings.txtwith the added nondeterminism check that was in the original PR.