[PATCH] D37905: [libclang, bindings]: add spelling location

[Masud Rahman via Phabricator via cfe-commits]

frutiger closed this revision.
frutiger added a comment.

@compnerd I do now.  Landed in https://reviews.llvm.org/rL316278.


https://reviews.llvm.org/D37905



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37905: [libclang, bindings]: add spelling location

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd accepted this revision.
compnerd added a comment.

@frutiger you have commit rights now right?


https://reviews.llvm.org/D37905



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37905: [libclang, bindings]: add spelling location

[Masud Rahman via Phabricator via cfe-commits]

frutiger updated this revision to Diff 119191.
frutiger added a comment.

Added 'Location' to '__all__'.


https://reviews.llvm.org/D37905

Files:
  bindings/python/clang/cindex.py
  bindings/python/tests/cindex/test_location.py
  tools/libclang/CXSourceLocation.cpp

Index: tools/libclang/CXSourceLocation.cpp
===
--- tools/libclang/CXSourceLocation.cpp
+++ tools/libclang/CXSourceLocation.cpp
@@ -318,8 +318,7 @@
   
   const SourceManager  =
   *static_cast(location.ptr_data[0]);
-  // FIXME: This should call SourceManager::getSpellingLoc().
-  SourceLocation SpellLoc = SM.getFileLoc(Loc);
+  SourceLocation SpellLoc = SM.getSpellingLoc(Loc);
   std::pair LocInfo = SM.getDecomposedLoc(SpellLoc);
   FileID FID = LocInfo.first;
   unsigned FileOffset = LocInfo.second;
Index: bindings/python/tests/cindex/test_location.py
===
--- bindings/python/tests/cindex/test_location.py
+++ bindings/python/tests/cindex/test_location.py
@@ -93,3 +93,10 @@
 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
Index: bindings/python/clang/cindex.py
===
--- bindings/python/clang/cindex.py
+++ bindings/python/clang/cindex.py
@@ -211,25 +211,45 @@
 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):
@@ -249,25 +269,73 @@
 """
 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 

[PATCH] D37905: [libclang, bindings]: add spelling location

[Johann Klähn via Phabricator via cfe-commits]

jklaehn added inline comments.



Comment at: bindings/python/clang/cindex.py:214
 
+class Location(object):
+"""A Location is a specific kind of source location.  A SourceLocation

Can you also add `Location` to `__all__`?


https://reviews.llvm.org/D37905



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37905: [libclang, bindings]: add spelling location

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd accepted this revision.
compnerd added inline comments.
This revision is now accepted and ready to land.



Comment at: bindings/python/clang/cindex.py:320
+return self._get_spelling()['offset']
 
 def __eq__(self, other):

frutiger wrote:
> compnerd wrote:
> > 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.
> 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?
Yeah, I think thats a great approach.


https://reviews.llvm.org/D37905



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37905: [libclang, bindings]: add spelling location

[Masud Rahman via Phabricator via cfe-commits]

frutiger updated this revision to Diff 117041.
frutiger added a comment.

Updated as per review.


https://reviews.llvm.org/D37905

Files:
  bindings/python/clang/cindex.py
  bindings/python/tests/cindex/test_location.py
  tools/libclang/CXSourceLocation.cpp

Index: tools/libclang/CXSourceLocation.cpp
===
--- tools/libclang/CXSourceLocation.cpp
+++ tools/libclang/CXSourceLocation.cpp
@@ -318,8 +318,7 @@
   
   const SourceManager  =
   *static_cast(location.ptr_data[0]);
-  // FIXME: This should call SourceManager::getSpellingLoc().
-  SourceLocation SpellLoc = SM.getFileLoc(Loc);
+  SourceLocation SpellLoc = SM.getSpellingLoc(Loc);
   std::pair LocInfo = SM.getDecomposedLoc(SpellLoc);
   FileID FID = LocInfo.first;
   unsigned FileOffset = LocInfo.second;
Index: bindings/python/tests/cindex/test_location.py
===
--- bindings/python/tests/cindex/test_location.py
+++ bindings/python/tests/cindex/test_location.py
@@ -93,3 +93,10 @@
 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
Index: bindings/python/clang/cindex.py
===
--- bindings/python/clang/cindex.py
+++ bindings/python/clang/cindex.py
@@ -211,25 +211,45 @@
 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):
@@ -249,25 +269,73 @@
 """
 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 = 

[PATCH] D37905: [libclang, bindings]: add spelling location

[Masud Rahman via Phabricator via cfe-commits]

frutiger added inline comments.



Comment at: bindings/python/clang/cindex.py:320
+return self._get_spelling()['offset']
 
 def __eq__(self, other):

compnerd wrote:
> 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.
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?


https://reviews.llvm.org/D37905



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37905: [libclang, bindings]: add spelling location

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd requested changes to this revision.
compnerd added a comment.
This revision now requires changes to proceed.

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.




Comment at: bindings/python/clang/cindex.py:320
+return self._get_spelling()['offset']
 
 def __eq__(self, other):

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.



Comment at: tools/libclang/CXSourceLocation.cpp:321
   *static_cast(location.ptr_data[0]);
   // FIXME: This should call SourceManager::getSpellingLoc().
+  SourceLocation SpellLoc = SM.getSpellingLoc(Loc);

Remove the FIXME please.


https://reviews.llvm.org/D37905



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37905: [libclang, bindings]: add spelling location

[Masud Rahman via Phabricator via cfe-commits]

frutiger added a comment.

//Friendly poke!//


https://reviews.llvm.org/D37905



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37905: [libclang, bindings]: add spelling location

[Masud Rahman via Phabricator via cfe-commits]

frutiger added a comment.

**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.


https://reviews.llvm.org/D37905



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37905: [libclang, bindings]: add spelling location

[Masud Rahman via Phabricator via cfe-commits]

frutiger created this revision.

o) Enhance 'CursorLocation' with four additional properties that can

  retrieve spelling location information.

o) Update the implementation to use 'clang_getExpansionLocation'

  instead of the deprecated 'clang_getInstantiationLocation', which
  has been present since 2011.

o) Update the implementation of 'clang_getSpellingLocation' to actually

  obtain spelling location instead of file location.


https://reviews.llvm.org/D37905

Files:
  bindings/python/clang/cindex.py
  bindings/python/tests/cindex/test_location.py
  tools/libclang/CXSourceLocation.cpp

Index: tools/libclang/CXSourceLocation.cpp
===
--- tools/libclang/CXSourceLocation.cpp
+++ tools/libclang/CXSourceLocation.cpp
@@ -319,7 +319,7 @@
   const SourceManager  =
   *static_cast(location.ptr_data[0]);
   // FIXME: This should call SourceManager::getSpellingLoc().
-  SourceLocation SpellLoc = SM.getFileLoc(Loc);
+  SourceLocation SpellLoc = SM.getSpellingLoc(Loc);
   std::pair LocInfo = SM.getDecomposedLoc(SpellLoc);
   FileID FID = LocInfo.first;
   unsigned FileOffset = LocInfo.second;
Index: bindings/python/tests/cindex/test_location.py
===
--- bindings/python/tests/cindex/test_location.py
+++ bindings/python/tests/cindex/test_location.py
@@ -93,3 +93,10 @@
 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.line == 2
Index: bindings/python/clang/cindex.py
===
--- bindings/python/clang/cindex.py
+++ bindings/python/clang/cindex.py
@@ -217,19 +217,48 @@
 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_data = None
+_spelling_data  = None
+
+def _get_expansion(self):
+if self._expansion_data is None:
+expansion_file   = c_object_p()
+expansion_line   = c_uint()
+expansion_column = c_uint()
+expansion_offset = c_uint()
+conf.lib.clang_getExpansionLocation(self,
+byref(expansion_file),
+byref(expansion_line),
+byref(expansion_column),
+byref(expansion_offset))
+
+self._expansion_data = {
+'file':   File(expansion_file) if expansion_file else None,
+'line':   int(expansion_line.value),
+'column': int(expansion_column.value),
+'offset': int(expansion_offset.value),
+}
+return self._expansion_data
+
+def _get_spelling(self):
+if self._spelling_data is None:
+spelling_file   = c_object_p()
+spelling_line   = c_uint()
+spelling_column = c_uint()
+spelling_offset = c_uint()
+conf.lib.clang_getSpellingLocation(self,
+   byref(spelling_file),
+   byref(spelling_line),
+   byref(spelling_column),
+   byref(spelling_offset))
+
+self._spelling_data = {
+'file':   File(spelling_file) if spelling_file else None,
+'line':   int(spelling_line.value),
+'column': int(spelling_column.value),
+'offset': int(spelling_offset.value),
+}
+return self._spelling_data
 
 @staticmethod
 def from_position(tu, file, line, column):
@@ -252,22 +281,42 @@
 @property
 def file(self):
 """Get the file represented by this source location."""
-return self._get_instantiation()[0]
+return self._get_expansion()['file']
 
 @property
 def line(self):
 """Get the line represented by this source location."""
-return self._get_instantiation()[1]
+return self._get_expansion()['line']
 
 @property
 def column(self):