johannes added a comment.

In https://reviews.llvm.org/D36176#830341, @arphaman wrote:

> 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


https://reviews.llvm.org/D36176



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to