Fix to the computation of the rightmost descendant. Prevents root
nodes from being mapped if they are already mapped. This only makes a
difference when we compare AST Nodes other than entire translation units, a
feature which still has to be tested.
Details
Diff Detail
- Build Status
Buildable 8920 Build 8920: arc lint + arc unit
Event Timeline
Is this an NFC patch or does it have an impact on the produced results? If yes, can you come up with a test that ensures that the new results are verified?
include/clang/Tooling/ASTDiff/ASTDiff.h | ||
---|---|---|
96 | It might be better to add a move constructor which would disable copy constructors. |
include/clang/Tooling/ASTDiff/ASTDiff.h | ||
---|---|---|
96 | ASTDiff::getMapped internally uses the address of the SyntaxTree that is passed as parameter. This is quite bad. I will change ASTDiff::getMapped to accept a boolean or split it into two methods |
include/clang/Tooling/ASTDiff/ASTDiff.h | ||
---|---|---|
96 | Can't you just use the TreeImpl pointer value? That should be the same even after you've moved SyntaxTree with the move constructor. |
include/clang/Tooling/ASTDiff/ASTDiff.h | ||
---|---|---|
96 | ok perfect |
LGTM. Although I'm not 100% sure it's fully NFC, so if you can't come up with a test please justify why.
There are changes that affect the output.
Firstly the computation of the rightmost descendant; the added assertion fails every time with the old version.
Secondly, the change around line 811 prevents root nodes from being mapped if they are already mapped. When comparing translation units this happens only if the two trees are identical (in this case the output would not change). Otherwise, it would require some change in the client to demonstrate how the behaviour changed. One needs to compare AST Nodes other than entire translation units. At some point there will be something that uses this feature so we cover it too (or remove the feature). So I'd keep it like this
It might be better to add a move constructor which would disable copy constructors.