Merge pull request #544 from kleintom/destructor

Fix up destructor name tokenization in clang.

dxr/plugins/clang/dxr-index.cpp

@@ -249,11 +249,34 @@ class IndexConsumer : public ASTConsumer,
   }
 
   // Return the location right after the token at `loc` finishes.
-  SourceLocation afterToken(SourceLocation loc) {
+  SourceLocation afterToken(SourceLocation loc, const std::string& tokenHint = "") {
+    // TODO: Would it be safe to just change this function to be
+    //    return loc.getLocWithOffset(tokenHint.size());
+    // in the cases where we already know the full name in the caller?
+    // In the meantime we're only using tokenHint for its first character: the
+    // lexer treats '~' as a token in some cases and not in others (due to the
+    // ambiguity between whether '~X();' means destructor decl or one's
+    // complement on the return of a function), so we help things along by just
+    // always starting one after the '~'.
+    if (tokenHint.size() > 1 && tokenHint[0] == '~')
+      loc = loc.getLocWithOffset(1);
     // TODO: Perhaps, at callers, pass me begin if !end.isValid().
     return Lexer::getLocForEndOfToken(loc, 0, sm, features);
   }
 
+  // Record the correct "locend" for name (when name is the name of a destructor
+  // it works around some inconsistencies).
+  void recordLocEndForName(const std::string& name,
+                           SourceLocation beginLoc, SourceLocation endLoc) {
+    // Some versions (or settings or whatever) of clang consider '~MyClass' to
+    // be two tokens, others just one, so we just always let afterToken take
+    // care of the '~' when there is one.
+    if (name.size() > 1 && name[0] == '~')
+      recordValue("locend", locationToString(afterToken(beginLoc, name)));
+    else
+      recordValue("locend", locationToString(afterToken(endLoc)));
+  }
+
   // This is a wrapper around NamedDecl::getQualifiedNameAsString.
   // It produces more qualified output to distinguish several cases
   // which would otherwise be ambiguous.
@@ -418,10 +441,12 @@ class IndexConsumer : public ASTConsumer,
 
     beginRecord("decldef", decl->getLocation());  // Assuming this is an
                                                   // expansion location.
-    recordValue("name", decl->getNameAsString());
+    std::string name = decl->getNameAsString();
+    recordValue("name", name);
     recordValue("qualname", getQualifiedName(*def));
     recordValue("loc", locationToString(decl->getLocation()));
-    recordValue("locend", locationToString(afterToken(decl->getLocation())));
+    recordValue("locend",
+                locationToString(afterToken(decl->getLocation(), name)));
     recordValue("defloc", locationToString(def->getLocation()));
     if (kind)
       recordValue("kind", kind);
@@ -545,8 +570,9 @@ class IndexConsumer : public ASTConsumer,
         args.erase(1, 2);
       args += ")";
       recordValue("args", args);
-      recordValue("loc", locationToString(d->getNameInfo().getBeginLoc()));
-      recordValue("locend", locationToString(afterToken(d->getNameInfo().getEndLoc())));
+      SourceLocation beginLoc = d->getNameInfo().getBeginLoc();
+      recordValue("loc", locationToString(beginLoc));
+      recordLocEndForName(functionName, beginLoc, d->getNameInfo().getEndLoc());
       printScope(d);
       *out << std::endl;
 
@@ -815,13 +841,14 @@ class IndexConsumer : public ASTConsumer,
     if (!interestingLocation(d->getLocation()) || !interestingLocation(refLoc))
       return;
     SourceLocation nonMacroRefLoc = escapeMacros(refLoc);
+    std::string name = d->getNameAsString();
     beginRecord("ref", nonMacroRefLoc);
     recordValue("defloc", locationToString(d->getLocation()));
     recordValue("loc", locationToString(nonMacroRefLoc));
-    recordValue("locend", locationToString(afterToken(escapeMacros(end))));
+    recordLocEndForName(name, nonMacroRefLoc, escapeMacros(end));
     if (kind)
       recordValue("kind", kind);
-    recordValue("name", d->getNameAsString());
+    recordValue("name", name);
     recordValue("qualname", getQualifiedName(*d));
     *out << std::endl;
   }

dxr/plugins/clang/tests/test_menus/code/BaseTypes.h

@@ -3,11 +3,20 @@
 
 class BaseClass {
 public:
+  virtual ~BaseClass() {}
   virtual void virtualFunc() { }
   virtual void pVirtualFunc() = 0;
 };
 
 struct BaseStruct {
 };
 
+// I'm giving this class a single-letter name so that when we test for
+// destructor extents it'll be easy to tell if we skipped over the '~'
+// incorrectly.
+class Z {
+public:
+  ~Z();
+};
+
 #endif

dxr/plugins/clang/tests/test_menus/code/Z.cpp

@@ -0,0 +1,3 @@
+#include "BaseTypes.h"
+
+Z::~Z() {}

dxr/plugins/clang/tests/test_menus/code/main.cpp

@@ -18,6 +18,9 @@ int main(int argc, char* argv[]) {
   der->virtualFunc();
   delete der;
 
+  Z zz;
+  zz.~Z();
+
   return 1;
 }
 

dxr/plugins/clang/tests/test_menus/code/makefile

@@ -1,7 +1,10 @@
 all: code
 
-code:
-	$(CXX) main.cpp -o code
+code: Z.o main.o
+	$(CXX) -o $@ $^
+
+%.o: %.cpp
+	$(CXX) -c -o $@ $^
 
 clean:
 	rm -rf code *.o

dxr/plugins/clang/tests/test_menus/test_menus.py

@@ -283,3 +283,28 @@ def test_override_pure_overridden(self):
                 'pVirtualFunc',
                 {'html': 'Find overridden',
                  'href': '/code/search?q=%2Boverridden%3ADerivedClass%3A%3ApVirtualFunc%28%29'})
+
+    def test_destructor_declaration(self):
+        """Make sure we link the full destructor name and not just the '~' on a
+        destructor declaration, which can be easily confused for one's
+        complement on the result of a function call."""
+        menu_on(self.source_page('BaseTypes.h'),
+                '~Z',
+                {'html': 'Find declarations',
+                 'href': '/code/search?q=%2Bfunction-decl%3AZ%3A%3A%7EZ%28%29'})
+
+    def test_destructor_def(self):
+        """Make sure we link the full destructor name and not just the '~' on a
+        destructor def."""
+        menu_on(self.source_page('Z.cpp'),
+                '~Z',
+                {'html': 'Find callers',
+                 'href': '/code/search?q=%2Bcallers%3AZ%3A%3A%7EZ%28%29'})
+
+    def test_destructor_call(self):
+        """Make sure we link the full destructor name and not just the '~' on a
+        destructor call."""
+        menu_on(self.source_page('main.cpp'),
+                '~Z',
+                {'html': 'Find references',
+                 'href': '/code/search?q=%2Bfunction-ref%3AZ%3A%3A%7EZ%28%29'})

dxr/static_unhashed/js/context_menu.js

@@ -6,7 +6,16 @@ $(function() {
     // Get the file content container
     var fileContainer = $('#file'),
         queryField = $('#query'),
-        contentContainer = $('#content');
+        contentContainer = $('#content'),
+        nonWordCharRE = /[^A-Z0-9_]/i;
+
+    if (['cpp', 'cc', 'cxx', 'h', 'hxx', 'hpp'].indexOf(
+          extension(window.location.pathname).toLowerCase()) > -1) {
+        // C++ files should include '~' as a valid name character.  (This
+        // doesn't cause us to accidentally scoop up '~' operators because those
+        // '~'s aren't in the same html node as the thing they operate on).
+        nonWordCharRE = /[^A-Z0-9_~]/i;
+    }
 
     /**
      * Highlight, or remove highlighting from, all symbols with the same class
@@ -114,8 +123,8 @@ $(function() {
             var offset = selection.focusOffset,
                 node = selection.anchorNode,
                 selectedTxtString = node.nodeValue,
-                startIndex = selectedTxtString.regexLastIndexOf(/[^A-Z0-9_]/i, offset) + 1,
-                endIndex = selectedTxtString.regexIndexOf(/[^A-Z0-9_]/i, offset),
+                startIndex = selectedTxtString.regexLastIndexOf(nonWordCharRE, offset) + 1,
+                endIndex = selectedTxtString.regexIndexOf(nonWordCharRE, offset),
                 word = '';
 
             // If the regex did not find a start index, start from index 0

dxr/static_unhashed/js/utils.js

@@ -73,3 +73,15 @@ function hideOptionsAndHelp() {
     $('.select-options, .sf-select-options').hide();
     hideHelp();
 }
+
+/**
+ * Return the extension of the given path, or '' if no extension exists.
+ */
+function extension(path) {
+    var file = path.substring(path.lastIndexOf('/') + 1),
+        lastDotPos = file.lastIndexOf('.');
+    if (lastDotPos === -1) {
+        return '';
+    }
+    return file.substring(lastDotPos + 1);
+}