[PATCH] D36176: [clang-diff] Fix some errors and inconsistencies

Johannes Altmanninger via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Aug 3 06:17:25 PDT 2017


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





More information about the cfe-commits mailing list