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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
