[PATCH] D34329: [GSoC] Clang AST diffing

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

The API looks better IMHO. Some more comments:



================
Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:57
+/// Within a tree, this identifies a node by its preorder offset.
+using NodeId = int;
+
----------------
I think that it's better to make make `NodeId` a structure as well and make `InvalidNodeId` a private member. I suggest the following interface for `NodeId`:

```
struct NodeId {
private:
  static const int InvalidNodeId; 
public:
  int Id;

  NodeId() : Id(InvalidNodeId) { }
  NodeId(int Id) : Id(Id) { }
  
  bool isValid() const { return Id != InvalidNodeId; }
  bool isInvalid() const { return Id == InvalidNodeId; }
};
```


This way you'll get rid of a couple of variable initializations that use `InvalidNodeId`. You also won't need to call the `memset` when creating the unique pointer array of `NodeId`s.


================
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:33
+/// Supports fast insertion and lookup.
+class Mapping {
+public:
----------------
Mapping should use the C++ move semantics IMHO.


================
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:35
+public:
+  bool Done = false;
+  Mapping() = default;
----------------
This field is used only in `TreeComparator`, so you might as well move it there and rename to something like `IsMappingDone`.


================
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:318
+    return "";
+  if (auto *X = DTN.get<QualType>())
+    llvm_unreachable("Types are not included.\n");
----------------
We don't really need this check. Let's use just one unreachable here.


================
Comment at: lib/Tooling/ASTDiff/ASTDiff.cpp:638
+                                      NodeId Id2) const {
+  const Node &N1 = T1.getNode(Id1);
+  const Node &N2 = T2.getNode(Id2);
----------------
I think this function would would be clearer and faster (you'll be able to avoid redundant work) if you use early exists for the main conditions of the `return`, e.g.:

`if (M.hasSrc(Id1) || M.hasDst(Id2)) return false; // Both nodes must not be mapped.`

And so on. 


https://reviews.llvm.org/D34329