[clang-diff] Use the relative name for NamedDecls

[johannes]

Summary

If the node is within a class or namespace, do not use its full
qualified name, but strip the namespace prefix.

[johannes]

update

[johannes]

parent changed

[klimek]

+Ben who I believe had written a similar function once.

lib/Tooling/ASTDiff/ASTDiff.cpp

370–371

"If the is ..." doesn't parse.

[bkramer]

There's some similar code in tools/clang/lib/Tooling/Core/Lookup.cpp, it might make sense to share it. Otherwise this looks good.

lib/Tooling/ASTDiff/ASTDiff.cpp

384

Val.startswith(ContextPrefix)

[johannes]

substr

[arphaman]

Please rebase, it doesn't apply cleanly anymore.

[johannes]

rebase

[arphaman]

lib/Tooling/ASTDiff/ASTDiff.cpp

375

You don't need to check both NamespaceDecl and TagDecl, since you can just do one if with a NamedDecl.

[johannes]

only use Enums as namespace prefix in C++11

[johannes]

lib/Tooling/ASTDiff/ASTDiff.cpp

375

FunctionDecl should not be used for example

[arphaman]

LG

lib/Tooling/ASTDiff/ASTDiff.cpp

375

Oh yeah, makes sense.

include/clang/Tooling/ASTDiff/ASTDiff.h

private:
   std::unique_ptr<Impl> DiffImpl;
 };
 
 /// SyntaxTree objects represent subtrees of the AST.
 /// They can be constructed from any Decl or Stmt.
 class SyntaxTree {
 public:
   /// Constructs a tree from a translation unit.
-  SyntaxTree(const ASTContext &AST);
+  SyntaxTree(ASTContext &AST);
   /// Constructs a tree from any AST node.
   template <class T>
-  SyntaxTree(T *Node, const ASTContext &AST)
+  SyntaxTree(T *Node, ASTContext &AST)
       : TreeImpl(llvm::make_unique<Impl>(this, Node, AST)) {}
   SyntaxTree(SyntaxTree &&Other) = default;
   ~SyntaxTree();
 
   const ASTContext &getASTContext() const;
   StringRef getFilename() const;
 
   int getSize() const;

lib/Tooling/ASTDiff/ASTDiff.cpp

private:
   const ComparisonOptions &Options;
 
   friend class ZhangShashaMatcher;
 };
 
 /// Represents the AST of a TranslationUnit.
 class SyntaxTree::Impl {
 public:
-  /// Constructs a tree from the entire translation unit.
-  Impl(SyntaxTree *Parent, const ASTContext &AST);
   /// Constructs a tree from an AST node.
-  Impl(SyntaxTree *Parent, Decl *N, const ASTContext &AST);
-  Impl(SyntaxTree *Parent, Stmt *N, const ASTContext &AST);
+  Impl(SyntaxTree *Parent, Decl *N, ASTContext &AST);
+  Impl(SyntaxTree *Parent, Stmt *N, ASTContext &AST);
   template <class T>
   Impl(SyntaxTree *Parent,
        typename std::enable_if<std::is_base_of<Stmt, T>::value, T>::type *Node,
-       const ASTContext &AST)
+       ASTContext &AST)
       : Impl(Parent, dyn_cast<Stmt>(Node), AST) {}
   template <class T>
   Impl(SyntaxTree *Parent,
        typename std::enable_if<std::is_base_of<Decl, T>::value, T>::type *Node,
-       const ASTContext &AST)
+       ASTContext &AST)
       : Impl(Parent, dyn_cast<Decl>(Node), AST) {}
 
   SyntaxTree *Parent;
-  const ASTContext &AST;
+  ASTContext &AST;
   std::vector<NodeId> Leaves;
   // Maps preorder indices to postorder ones.
   std::vector<int> PostorderIds;
   std::vector<NodeId> NodesBfs;
 
   int getSize() const { return Nodes.size(); }
   NodeId getRootId() const { return 0; }
   PreorderIterator begin() const { return getRootId(); }
   PreorderIterator end() const { return getSize(); }
 
   const Node &getNode(NodeId Id) const { return Nodes[Id]; }
   Node &getMutableNode(NodeId Id) { return Nodes[Id]; }
   bool isValidNodeId(NodeId Id) const { return Id >= 0 && Id < getSize(); }
   void addNode(Node &N) { Nodes.push_back(N); }
   int getNumberOfDescendants(NodeId Id) const;
   bool isInSubtree(NodeId Id, NodeId SubtreeRoot) const;
   int findPositionInParent(NodeId Id, bool Shifted = false) const;
 
+  std::string getRelativeName(const NamedDecl *ND,
+                              const DeclContext *Context) const;
+  std::string getRelativeName(const NamedDecl *ND) const;
+
   std::string getNodeValue(NodeId Id) const;
   std::string getNodeValue(const Node &Node) const;
   std::string getDeclValue(const Decl *D) const;
   std::string getStmtValue(const Stmt *S) const;
 
 private:
   /// Nodes in preorder.
   std::vector<Node> Nodes;
bool TraverseStmt(Stmt *S) {
     RecursiveASTVisitor<PreorderVisitor>::TraverseStmt(S);
     PostTraverse(SavedState);
     return true;
   }
   bool TraverseType(QualType T) { return true; }
 };
 } // end anonymous namespace
 
-SyntaxTree::Impl::Impl(SyntaxTree *Parent, Decl *N, const ASTContext &AST)
+SyntaxTree::Impl::Impl(SyntaxTree *Parent, Decl *N, ASTContext &AST)
     : Parent(Parent), AST(AST) {
   NodeCountVisitor NodeCounter(*this);
   NodeCounter.TraverseDecl(N);
   Nodes.resize(NodeCounter.Count);
   PreorderVisitor PreorderWalker(*this);
   PreorderWalker.TraverseDecl(N);
   initTree();
 }
 
-SyntaxTree::Impl::Impl(SyntaxTree *Parent, Stmt *N, const ASTContext &AST)
+SyntaxTree::Impl::Impl(SyntaxTree *Parent, Stmt *N, ASTContext &AST)
     : Parent(Parent), AST(AST) {
   NodeCountVisitor NodeCounter(*this);
   NodeCounter.TraverseStmt(N);
   Nodes.resize(NodeCounter.Count);
   PreorderVisitor PreorderWalker(*this);
   PreorderWalker.TraverseStmt(N);
   initTree();
 }
for (size_t I = 0, E = Siblings.size(); I < E; ++I) {
     if (Siblings[I] == Id) {
       Position += I;
       return Position;
     }
   }
   llvm_unreachable("Node not found in parent's children.");
 }
 
+// Returns the qualified name of ND. If it is subordinate to Context,
+// then the prefix of the latter is removed from the returned value.
+std::string
+SyntaxTree::Impl::getRelativeName(const NamedDecl *ND,
+                                  const DeclContext *Context) const {
+  std::string ContextPrefix;
+  if (auto *Namespace = dyn_cast<NamedDecl>(Context))
+    ContextPrefix = Namespace->getQualifiedNameAsString();
+  else if (auto *Record = dyn_cast<RecordDecl>(Context))
+    ContextPrefix = Record->getQualifiedNameAsString();
+  else if (AST.getLangOpts().CPlusPlus11)
+    if (auto *Tag = dyn_cast<TagDecl>(Context))
+      ContextPrefix = Tag->getQualifiedNameAsString();
+  std::string Val = ND->getQualifiedNameAsString();
+  // Strip the qualifier, if Val refers to somthing in the current scope.
+  // But leave one leading ':' in place, so that we know that this is a
+  // relative path.
+  if (!ContextPrefix.empty() && StringRef(Val).startswith(ContextPrefix))
+    Val = Val.substr(ContextPrefix.size() + 1);
+  return Val;
+}
+
+std::string SyntaxTree::Impl::getRelativeName(const NamedDecl *ND) const {
+  return getRelativeName(ND, ND->getDeclContext());
+}
+
+static const DeclContext *getEnclosingDeclContext(ASTContext &AST,
+                                                  const Stmt *S) {
+  while (S) {
+    const auto &Parents = AST.getParents(*S);
+    if (Parents.empty())
+      return nullptr;
+    const auto &P = Parents[0];
+    if (const auto *D = P.get<Decl>())
+      return D->getDeclContext();
+    S = P.get<Stmt>();
+  }
+  llvm_unreachable("Could not find Decl ancestor.");
+}
+
 std::string SyntaxTree::Impl::getNodeValue(NodeId Id) const {
   return getNodeValue(getNode(Id));
 }
 
 std::string SyntaxTree::Impl::getNodeValue(const Node &N) const {
   const DynTypedNode &DTN = N.ASTNode;
   if (auto *S = DTN.get<Stmt>())
     return getStmtValue(S);
   if (auto *D = DTN.get<Decl>())
     return getDeclValue(D);
   llvm_unreachable("Fatal: unhandled AST node.\n");
 }
 
 std::string SyntaxTree::Impl::getDeclValue(const Decl *D) const {
   std::string Value;
   PrintingPolicy TypePP(AST.getLangOpts());
   TypePP.AnonymousTagLocations = false;
 
   if (auto *V = dyn_cast<ValueDecl>(D)) {
-    Value += V->getQualifiedNameAsString() + "(" +
-             V->getType().getAsString(TypePP) + ")";
+    Value += getRelativeName(V) + "(" + V->getType().getAsString(TypePP) + ")";
     if (auto *C = dyn_cast<CXXConstructorDecl>(D)) {
       for (auto *Init : C->inits()) {
         if (!Init->isWritten())
           continue;
         if (Init->isBaseInitializer()) {
           Value += Init->getBaseClass()->getCanonicalTypeInternal().getAsString(
               TypePP);
         } else if (Init->isDelegatingInitializer()) {
           Value += C->getNameAsString();
         } else {
           assert(Init->isAnyMemberInitializer());
-          Value += Init->getMember()->getQualifiedNameAsString();
+          Value += getRelativeName(Init->getMember());
         }
         Value += ",";
       }
     }
     return Value;
   }
   if (auto *N = dyn_cast<NamedDecl>(D))
-    Value += N->getQualifiedNameAsString() + ";";
+    Value += getRelativeName(N) + ";";
   if (auto *T = dyn_cast<TypedefNameDecl>(D))
     return Value + T->getUnderlyingType().getAsString(TypePP) + ";";
   if (auto *T = dyn_cast<TypeDecl>(D))
     if (T->getTypeForDecl())
       Value +=
           T->getTypeForDecl()->getCanonicalTypeInternal().getAsString(TypePP) +
           ";";
   if (auto *U = dyn_cast<UsingDirectiveDecl>(D))
     return U->getNominatedNamespace()->getName();
   if (auto *A = dyn_cast<AccessSpecDecl>(D)) {
     CharSourceRange Range(A->getSourceRange(), false);
     return Lexer::getSourceText(Range, AST.getSourceManager(),
                                 AST.getLangOpts());
   }
   return Value;
 }
 
 std::string SyntaxTree::Impl::getStmtValue(const Stmt *S) const {
   if (auto *U = dyn_cast<UnaryOperator>(S))
     return UnaryOperator::getOpcodeStr(U->getOpcode());
   if (auto *B = dyn_cast<BinaryOperator>(S))
     return B->getOpcodeStr();
   if (auto *M = dyn_cast<MemberExpr>(S))
-    return M->getMemberDecl()->getQualifiedNameAsString();
+    return getRelativeName(M->getMemberDecl());
   if (auto *I = dyn_cast<IntegerLiteral>(S)) {
     SmallString<256> Str;
     I->getValue().toString(Str, /*Radix=*/10, /*Signed=*/false);
     return Str.str();
   }
   if (auto *F = dyn_cast<FloatingLiteral>(S)) {
     SmallString<256> Str;
     F->getValue().toString(Str);
     return Str.str();
   }
   if (auto *D = dyn_cast<DeclRefExpr>(S))
-    return D->getDecl()->getQualifiedNameAsString();
+    return getRelativeName(D->getDecl(), getEnclosingDeclContext(AST, S));
   if (auto *String = dyn_cast<StringLiteral>(S))
     return String->getString();
   if (auto *B = dyn_cast<CXXBoolLiteralExpr>(S))
     return B->getValue() ? "true" : "false";
   return "";
 }
 
 /// Identifies a node in a subtree by its postorder offset, starting at 1.
ASTDiff::ASTDiff(SyntaxTree &T1, SyntaxTree &T2,
     : DiffImpl(llvm::make_unique<Impl>(*T1.TreeImpl, *T2.TreeImpl, Options)) {}
 
 ASTDiff::~ASTDiff() = default;
 
 NodeId ASTDiff::getMapped(const SyntaxTree &SourceTree, NodeId Id) const {
   return DiffImpl->getMapped(SourceTree.TreeImpl, Id);
 }
 
-SyntaxTree::SyntaxTree(const ASTContext &AST)
+SyntaxTree::SyntaxTree(ASTContext &AST)
     : TreeImpl(llvm::make_unique<SyntaxTree::Impl>(
           this, AST.getTranslationUnitDecl(), AST)) {}
 
 SyntaxTree::~SyntaxTree() = default;
 
 const ASTContext &SyntaxTree::getASTContext() const { return TreeImpl->AST; }
 
 const Node &SyntaxTree::getNode(NodeId Id) const {

test/Tooling/clang-diff-ast.cpp

 // RUN: clang-diff -ast-dump %s -- -std=c++11 | FileCheck %s
 
 
 // CHECK: {{^}}TranslationUnitDecl(0)
 // CHECK: {{^}} NamespaceDecl: test;(
 namespace test {
 
-// CHECK: {{^}} FunctionDecl: test::f(
+// CHECK: {{^}} FunctionDecl: :f(
 // CHECK: CompoundStmt(
 void f() {
   // CHECK: VarDecl: i(int)(
   // CHECK: IntegerLiteral: 1
   auto i = 1;
   // CHECK: FloatingLiteral: 1.5(
   auto r = 1.5;
   // CHECK: CXXBoolLiteralExpr: true(
   auto b = true;
   // CHECK: CallExpr(
   // CHECK-NOT: ImplicitCastExpr
-  // CHECK: DeclRefExpr: test::f(
+  // CHECK: DeclRefExpr: :f(
   f();
   // CHECK: UnaryOperator: ++(
   ++i;
   // CHECK: BinaryOperator: =(
   i = i;
 }
 
 } // end namespace test
 
 // CHECK: UsingDirectiveDecl: test(
 using namespace test;
 
 // CHECK: TypedefDecl: nat;unsigned int;(
 typedef unsigned nat;
 // CHECK: TypeAliasDecl: real;double;(
 using real = double;
 
 class Base {
 };
 
 // CHECK: CXXRecordDecl: X;X;(
 class X : Base {
   int m;
-  // CHECK: CXXMethodDecl: X::foo(const char *(int))(
+  // CHECK: CXXMethodDecl: :foo(const char *(int))(
   // CHECK: ParmVarDecl: i(int)(
   const char *foo(int i) {
     if (i == 0)
       // CHECK: StringLiteral: foo(
       return "foo";
     // CHECK-NOT: ImplicitCastExpr
     return 0;
   }
 
   // CHECK: AccessSpecDecl: public(
 public:
   int not_initialized;
   // All initialized members, bases and delegating initializers are are
   // appended, separated by commas.
-  // CHECK: CXXConstructorDecl: X::X(void (char, int))Base,X::m,(
-  X(char c, int) : Base(), m(0) {
-    // CHECK: MemberExpr: X::m(
+  // CHECK: CXXConstructorDecl: :X(void (char, int))Base,:m,(
+  X(char, int) : Base(), m(0) {
+    // CHECK: MemberExpr: :m(
     int x = m;
   }
-  // CHECK: CXXConstructorDecl: X::X(void (char))X,(
+  // CHECK: CXXConstructorDecl: :X(void (char))X,(
   X(char s) : X(s, 4) {}
 };
 
 #define M (void)1
 #define MA(a, b) (void)a, b
 // CHECK: FunctionDecl
 // CHECK-NEXT: CompoundStmt
 void macros() {

test/Tooling/clang-diff-basic.cpp

 // RUN: clang-diff -dump-matches %S/Inputs/clang-diff-basic-src.cpp %s -- | FileCheck %s
 
 // CHECK: Match TranslationUnitDecl{{.*}} to TranslationUnitDecl
 // CHECK: Match NamespaceDecl: src{{.*}} to NamespaceDecl: dst
 namespace dst {
 // CHECK-NOT: Match NamespaceDecl: src{{.*}} to NamespaceDecl: inner
 namespace inner {
 void foo() {
   // CHECK: Match IntegerLiteral: 321{{.*}} to IntegerLiteral: 322
   int x = 322;
 }
 }
 
-// CHECK: Match DeclRefExpr: src::foo{{.*}} to DeclRefExpr: dst::inner::foo
+// CHECK: Match DeclRefExpr: :foo{{.*}} to DeclRefExpr: :inner::foo
 void main() { inner::foo(); }
 
 // CHECK: Match StringLiteral: foo{{.*}} to StringLiteral: foo
 const char *b = "f" "o" "o";
 
 // unsigned is canonicalized to unsigned int
-// CHECK: Match TypedefDecl: src::nat;unsigned int;{{.*}} to TypedefDecl: dst::nat;unsigned int;
+// CHECK: Match TypedefDecl: :nat;unsigned int;{{.*}} to TypedefDecl: :nat;unsigned int;
 typedef unsigned nat;
 
-// CHECK: Match VarDecl: src::p(int){{.*}} to VarDecl: dst::prod(double)
-// CHECK: Update VarDecl: src::p(int){{.*}} to dst::prod(double)
+// CHECK: Match VarDecl: :p(int){{.*}} to VarDecl: :prod(double)
+// CHECK: Update VarDecl: :p(int){{.*}} to :prod(double)
 // CHECK: Match BinaryOperator: *{{.*}} to BinaryOperator: *
 double prod = 1 * 2 * 10;
 // CHECK: Update DeclRefExpr
 int squared = prod * prod;
 
 class X {
   const char *foo(int i) {
     if (i == 0)

test/Tooling/clang-diff-html.test

 // RUN: clang-diff -html %S/Inputs/clang-diff-basic-src.cpp %S/clang-diff-basic.cpp -- | FileCheck %s
 
 // CHECK: <pre><div id='L' class='code'><span id='L0' tid='R0' title='TranslationUnitDecl
 // CHECK-NEXT: 0 -> 0'>
 
 // match, update
 // CHECK: <span id='L[[L:[0-9]+]]' tid='R[[R:[0-9]+]]' title='NamespaceDecl
 // CHECK-NEXT: [[L]] -> [[R]]
 // CHECK-NEXT: src;' class='u'>namespace src {
 
 // match, move
 // CHECK: <span id='L[[L:[0-9]+]]' tid='R[[R:[0-9]+]]' title='FunctionDecl
 // CHECK-NEXT: [[L]] -> [[R]]
-// CHECK-NEXT: src::foo(void ())' class='u m'>void foo()
+// CHECK-NEXT: :foo(void ())' class='m'>void foo()
 
 // match
 // CHECK: <span id='L[[L:[0-9]+]]' tid='R[[R:[0-9]+]]' title='FunctionDecl
 // CHECK-NEXT: [[L]] -> [[R]]
-// CHECK-NEXT: src::main(void ())' class='u'>void main()
+// CHECK-NEXT: :main(void ())'>void main()
 
 // deletion
 // CHECK: <span id='L[[L:[0-9]+]]' tid='R-1' title='IntegerLiteral
 // CHECK-NEXT: [[L]] -> -1
 // CHECK-NEXT: 4' class='d'>4</span>
 
 // update + move
 // CHECK: 2' class='u m'>2</span>

test/Tooling/clang-diff-topdown.cpp

void f1()
   // Greedy approach - use the first matching subtree when there are multiple
   // identical subtrees.
   // CHECK: Match CompoundStmt(8) to CompoundStmt(8)
   // CHECK: Match CompoundStmt(9) to CompoundStmt(9)
   // CHECK: Match NullStmt(10) to NullStmt(10)
   {{;;}}
 }
 
+int x;
+
+namespace src {
+  int x;
+  int x1 = x + 1;
+  int x2 = ::x + 1;
+}
+
+class A { int x = 1 + 1; void f() { int x1 = x; } };
+
 #else
 
+
 void f1() {
 
   {{;}}
 
   {;}
 
   {{;;}}
   // CHECK-NOT: Match {{.*}} to CompoundStmt(11)
   // CHECK-NOT: Match {{.*}} to CompoundStmt(12)
   // CHECK-NOT: Match {{.*}} to NullStmt(13)
   {{;;}}
 
   // CHECK-NOT: Match {{.*}} to NullStmt(14)
   ;
 }
 
+int x;
+
+namespace dst {
+  int x;
+  // CHECK: Match DeclRefExpr: :x(17) to DeclRefExpr: :x(22)
+  int x1 = x + 1;
+  // CHECK: Match DeclRefExpr: x(21) to DeclRefExpr: x(26)
+  int x2 = ::x + 1;
+}
+
+class B {
+  // Only the class name changed; it is not included in the field value,
+  // therefore there is no update.
+  // CHECK: Match FieldDecl: :x(int)(24) to FieldDecl: :x(int)(29)
+  // CHECK-NOT: Update FieldDecl: :x(int)(24)
+  int x = 1+1;
+  void f() {
+    // CHECK: Match MemberExpr: :x(32) to MemberExpr: :x(37)
+    // CHECK-NOT: Update MemberExpr: :x(32)
+    int x1 = B::x;
+  }
+
+};
+
 #endif