[libclang, bindings]: add spelling location

[frutiger]

Summary

• Add two new properties to SourceLocation: expansion and spelling, allowing detailed source location information to be retrieved from a cursor.
• Update the implementation to use clang_getExpansionLocation instead of the deprecated clang_getInstantiationLocation, which has been present since 2011.
• Update the implementation of clang_getSpellingLocation to actually obtain spelling location instead of file location.

[frutiger]

Please note:

• this change means that versions of libclang built prior to the introduction of clang_getExpansionLocation will not work.
• this change alters the behavior of clang_getSpellingLocation to return the spelling location

I would appreciate advice on if either of these changes are acceptable. Thank you.

[frutiger]

Friendly poke!

[compnerd]

If I'm not mistaken, the change just means that the python bindings need a "newer" libclang, libclang's interfaces don't really change. I think that is acceptable.

bindings/python/clang/cindex.py

320

Does it make sense to introduce two new properties expansion and spelling and have the four fields be properties on those properties? It seems like it would be more pythonic.

tools/libclang/CXSourceLocation.cpp

321

Remove the FIXME please.

[frutiger]

bindings/python/clang/cindex.py

320

I agree, but I was concerned about breaking existing users that might be using the expansion properties directly on this object. Would marking them deprecated in the documentation suffice?

[frutiger]

Updated as per review.

[compnerd]

bindings/python/clang/cindex.py

320

Yeah, I think thats a great approach.

[jklaehn]

bindings/python/clang/cindex.py

214

Can you also add Location to __all__?

[frutiger]

Added 'Location' to 'all'.

[compnerd]

@frutiger you have commit rights now right?

[frutiger]

@compnerd I do now. Landed in rL316278.

bindings/python/clang/cindex.py

         assert isinstance(res, _CXString)
         return conf.lib.clang_getCString(res)
 
+class Location(object):
+    """A Location is a specific kind of source location.  A SourceLocation
+    refers to several kinds of locations (e.g.  spelling location vs. expansion
+    location)."""
+
+    def __init__(self, file, line, column, offset):
+        self._file   = File(file) if file else None
+        self._line   = int(line.value)
+        self._column = int(column.value)
+        self._offset = int(offset.value)
+
+
+    @property
+    def file(self):
+        """Get the file represented by this source location."""
+        return self._file
+
+    @property
+    def line(self):
+        """Get the line represented by this source location."""
+        return self._line
+
+    @property
+    def column(self):
+        """Get the column represented by this source location."""
+        return self._column
+
+    @property
+    def offset(self):
+        """Get the file offset represented by this source location."""
+        return self._offset
 
 class SourceLocation(Structure):
     """
     A SourceLocation represents a particular location within a source file.
     """
     _fields_ = [("ptr_data", c_void_p * 2), ("int_data", c_uint)]
-    _data = None
-
-    def _get_instantiation(self):
-        if self._data is None:
-            f, l, c, o = c_object_p(), c_uint(), c_uint(), c_uint()
-            conf.lib.clang_getInstantiationLocation(self, byref(f), byref(l),
-                    byref(c), byref(o))
-            if f:
-                f = File(f)
-            else:
-                f = None
-            self._data = (f, int(l.value), int(c.value), int(o.value))
-        return self._data
+    _expansion = None
+    _spelling = None
 
     @staticmethod
     def from_position(tu, file, line, column):

         """
         return conf.lib.clang_getLocationForOffset(tu, file, offset)
 
+    @property
+    def expansion(self):
+        """
+        The source location where then entity this object is referring to is
+        expanded.
+        """
+        if not self._expansion:
+            file   = c_object_p()
+            line   = c_uint()
+            column = c_uint()
+            offset = c_uint()
+            conf.lib.clang_getExpansionLocation(self,
+                                                byref(file),
+                                                byref(line),
+                                                byref(column),
+                                                byref(offset))
+
+            self._expansion = Location(file, line, column, offset)
+        return self._expansion
+
+    @property
+    def spelling(self):
+        """
+        The source location where then entity this object is referring to is
+        written.
+        """
+        if not self._spelling:
+            file   = c_object_p()
+            line   = c_uint()
+            column = c_uint()
+            offset = c_uint()
+            conf.lib.clang_getSpellingLocation(self,
+                                               byref(file),
+                                               byref(line),
+                                               byref(column),
+                                               byref(offset))
+
+            self._spelling = Location(file, line, column, offset)
+        return self._spelling
+
     @property
     def file(self):
-        """Get the file represented by this source location."""
-        return self._get_instantiation()[0]
+        """Get the file represented by this source location.
+
+        DEPRECATED: use expansion.file."""
+        return self.expansion.file
 
     @property
     def line(self):
-        """Get the line represented by this source location."""
-        return self._get_instantiation()[1]
+        """Get the line represented by this source location.
+
+        DEPRECATED: use expansion.line."""
+        return self.expansion.line
 
     @property
     def column(self):
-        """Get the column represented by this source location."""
-        return self._get_instantiation()[2]
+        """Get the column represented by this source location.
+
+        DEPRECATED: use expansion.column."""
+        return self.expansion.column
 
     @property
     def offset(self):
-        """Get the file offset represented by this source location."""
-        return self._get_instantiation()[3]
+        """Get the file offset represented by this source location.
+
+        DEPRECATED: use expansion.offset."""
+        return self.expansion.offset
 
     def __eq__(self, other):
         return conf.lib.clang_equalLocations(self, other)

     @property
     def location(self):
         """
-        Return the source location (the starting character) of the entity
-        pointed at by the cursor.
+        Return the source locations of the entity pointed at by the cursor.
         """
         if not hasattr(self, '_loc'):
             self._loc = conf.lib.clang_getCursorLocation(self)

   ("clang_getInclusions",
    [TranslationUnit, callbacks['translation_unit_includes'], py_object]),
 
-  ("clang_getInstantiationLocation",
+  ("clang_getExpansionLocation",
+   [SourceLocation, POINTER(c_object_p), POINTER(c_uint), POINTER(c_uint),
+    POINTER(c_uint)]),
+
+  ("clang_getSpellingLocation",
    [SourceLocation, POINTER(c_object_p), POINTER(c_uint), POINTER(c_uint),
     POINTER(c_uint)]),
 

bindings/python/tests/cindex/test_location.py

     location3 = SourceLocation.from_position(tu, file, 1, 6)
     range3 = SourceRange.from_locations(location1, location3)
     assert range1 != range3
+
+def test_spelling_location():
+    tu = get_tu('''#define ONE int one
+ONE;''')
+    one = get_cursor(tu, 'one')
+    assert one.location.spelling.line == 1
+    assert one.location.expansion.line == 2

tools/libclang/CXSourceLocation.cpp

   
   const SourceManager &SM =
   *static_cast<const SourceManager*>(location.ptr_data[0]);
-  // FIXME: This should call SourceManager::getSpellingLoc().
-  SourceLocation SpellLoc = SM.getFileLoc(Loc);
+  SourceLocation SpellLoc = SM.getSpellingLoc(Loc);
   std::pair<FileID, unsigned> LocInfo = SM.getDecomposedLoc(SpellLoc);
   FileID FID = LocInfo.first;
   unsigned FileOffset = LocInfo.second;