[PATCH] D34329: [GSoC] Clang AST diffing

[Manuel Klimek via Phabricator via cfe-commits]

klimek added inline comments.


================
Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:57
+/// Within a tree, this identifies a node by its preorder offset.
+using NodeId = int;
+
----------------
johannes wrote:
> arphaman wrote:
> > 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.
> Ok, I did it like this. 
> 
> Can I create a header file inside lib/Tooling/ASTDiff and include it from the public interface? This would help reduce the clutter.
> 
> Instead of NodeId we could also just use references / pointer types. I don't see any particularly good reason for choosing either one above the other.
> I guess indices make it more obvious how to compute the number of descendants and such. On the other hand, when using reference types, there is less boilerplate to write for loops.
No, but you can create a header ASTDiffInternal or somesuch next to the header in the public dir.


https://reviews.llvm.org/D34329