[libclang] Source range conversion

[jkorous]

Summary

I believe this is the correct conversion of CXSourceRange objects that are passed to libclang API and we should use it consistently everywhere. If it turns out that CXSourceRange objects can have multiple different semantics we should definitely introduce one type per semantic and fix libclang API to be type safe.

However, that is a bigger task and given the downsides (necessity to pass CXTranslationUnit everywhere, potential performance impact of the tokenization) we might end up with a completely different solution.

For context - test proposals + discussion:
https://reviews.llvm.org/D86840

[akyrtzi]

SourceRange cxloc::translateCharRangeToTokenRange(CXTranslationUnit TU, CXSourceRange R) is an expensive operation and I'm concerned it will be easy to start calling at places and introduce performance degradation.

Would it be viable to change return type of SourceRange translateCXSourceRange(CXSourceRange R) to CharSourceRange translateCXSourceRange(CXSourceRange R), assuming that the semantics of CXSourceRange is that it represents a character-based range? Then pass it along as CharSourceRange and if there are internal clang APIs that need to adjust to accept CharSourceRanges then we enhance them.

[jkorous]

@akyrtzi IIUC you are saying we should try to push the char-range semantics from libclang API as deep as possible to see if we could avoid this annoying conversion. I think that's a reasonable long-term strategy.
But I also think we'll inevitably run into some clang APIs that use SourceRange for valid reasons and we won't be able to change those. Some other clang APIs might require significant amount of work to be enhanced.

Ultimately, given this conversion isn't part of public libclang API and has explicit semantics, I would keep it - I'd just add a warning to its doc comment.

On practical level - I need this conversion for my other work and I can't go just now and refactor the Rewriter interface.
https://reviews.llvm.org/D86992

[akyrtzi]

In D86990#2252592, @jkorous wrote:

@akyrtzi IIUC you are saying we should try to push the char-range semantics from libclang API as deep as possible to see if we could avoid this annoying conversion. I think that's a reasonable long-term strategy.
But I also think we'll inevitably run into some clang APIs that use SourceRange for valid reasons and we won't be able to change those. Some other clang APIs might require significant amount of work to be enhanced.

Ultimately, given this conversion isn't part of public libclang API and has explicit semantics, I would keep it - I'd just add a warning to its doc comment.

On practical level - I need this conversion for my other work and I can't go just now and refactor the Rewriter interface.
https://reviews.llvm.org/D86992

AFAICT the Rewriter already supports CharSourceRange see from Rewriter.h:

/// getRangeSize - Return the size in bytes of the specified range if they
/// are in the same file.  If not, this returns -1.
int getRangeSize(SourceRange Range,
                 RewriteOptions opts = RewriteOptions()) const;
int getRangeSize(const CharSourceRange &Range,
                 RewriteOptions opts = RewriteOptions()) const;

/// getRewrittenText - Return the rewritten form of the text in the specified
/// range.  If the start or end of the range was unrewritable or if they are
/// in different buffers, this returns an empty string.
///
/// Note that this method is not particularly efficient.
std::string getRewrittenText(CharSourceRange Range) const;

/// getRewrittenText - Return the rewritten form of the text in the specified
/// range.  If the start or end of the range was unrewritable or if they are
/// in different buffers, this returns an empty string.
///
/// Note that this method is not particularly efficient.
std::string getRewrittenText(SourceRange Range) const {
  return getRewrittenText(CharSourceRange::getTokenRange(Range));
}

In fact CharSourceRange is a more natural type for the Rewriter to use since the Rewriter wants to resolve all ranges down to byte offsets in order to do its thing. And CXSourceRange is already character based, so if you end up doing:

CXSourceRange (character) -> (convert) SourceRange (token) -> Rewriter (convert to character) -> Rewriter (work with byte offset)

You add unnecessary conversion steps, compared to:

CXSourceRange (character) -> CharSourceRange (character) -> Rewriter (work with byte offset)

where no conversion is needed.

[jkorous]

You are absolutely right! I initially didn't realize the overloads of Rewriter methods can be of importance and haven't looked back since.

So, I'm adding the conversion you suggested and removing the illogical one to not confuse anyone.

Thanks!

[akyrtzi]

clang/tools/libclang/CXSourceLocation.h

74–79

"SourceRange" -> "CharSourceRange"

78

Minor nitpick but maybe rename to "translateCXRangeToCharRange", to make the name more clear?

[jkorous]

• fix the doc comment
• rename

[jkorous]

I forgot to add the magic "Differential revision:" prefix to the commit message :(

Landed as https://reviews.llvm.org/rGbaf3c77bd9f6baf60a09ef3625fef84080642b72

commit baf3c77bd9f6baf60a09ef3625fef84080642b72 (HEAD -> master, llvm.org/master)
Author: Jan Korous <[e-mail]>
Date:   Wed Sep 2 13:11:35 2020 -0700

    [libclang] Add translateCXRangeToCharRange conversion

    Add new conversion with clearly specified semantics.

    https://reviews.llvm.org/D86990

clang/tools/libclang/CIndex.cpp

if (IsTokenRange && EndLoc.isValid()) {
     EndLoc = EndLoc.getLocWithOffset(Length);
   }
 
   CXSourceRange Result = {
       {&SM, &LangOpts}, R.getBegin().getRawEncoding(), EndLoc.getRawEncoding()};
   return Result;
 }
 
+static void clang_tokenize_impl(ASTUnit *CXXUnit, SourceRange R,
+                                CXToken **Tokens, unsigned *NumTokens);
+
+SourceRange cxloc::translateCharRangeToTokenRange(CXTranslationUnit TU,
+                                                  CXSourceRange R) {
+  if (isNotUsableTU(TU)) {
+    LOG_BAD_TU(TU);
+    return {};
+  }
+
+  ASTUnit *CXXUnit = cxtu::getASTUnit(TU);
+  if (!CXXUnit)
+    return {};
+
+  CXToken *Tokens;
+  unsigned int NumTokens;
+  clang_tokenize_impl(
+      CXXUnit,
+      // FIXME: naturally tokenization doesn't expect a token range - we should
+      // use a distinct type.
+      SourceRange(SourceLocation::getFromRawEncoding(R.begin_int_data),
+                  SourceLocation::getFromRawEncoding(R.end_int_data)),
+      &Tokens, &NumTokens);
+
+  if (NumTokens == 0)
+    return {};
+
+  // Check that then begin of the range actually points to begin of some token.
+  const CXSourceRange FirstTokenRng = clang_getTokenExtent(TU, Tokens[0]);
+  if (FirstTokenRng.begin_int_data != R.begin_int_data)
+    return {};
+
+  const CXSourceRange LastTokenRng =
+      clang_getTokenExtent(TU, Tokens[NumTokens - 1]);
+  const SourceLocation LastTokenBegin =
+      SourceLocation::getFromRawEncoding(LastTokenRng.begin_int_data);
+
+  if (!LastTokenBegin.isValid())
+    return {};
+
+  return SourceRange(SourceLocation::getFromRawEncoding(R.begin_int_data),
+                     LastTokenBegin);
+}
+
 //===----------------------------------------------------------------------===//
 // Cursor visitor.
 //===----------------------------------------------------------------------===//
 
 static SourceRange getRawCursorExtent(CXCursor C);
 static SourceRange getFullCursorExtent(CXCursor C, SourceManager &SrcMgr);
 
 RangeComparisonResult CursorVisitor::CompareRegionOfInterest(SourceRange R) {
CXToken *clang_getToken(CXTranslationUnit TU, CXSourceLocation Location) {
 
   CXTokens.resize(1);
   CXToken *Token = static_cast<CXToken *>(llvm::safe_malloc(sizeof(CXToken)));
 
   memmove(Token, CXTokens.data(), sizeof(CXToken));
   return Token;
 }
 
+static void clang_tokenize_impl(ASTUnit *CXXUnit, SourceRange R,
+                                CXToken **Tokens, unsigned *NumTokens) {
+  if (!CXXUnit || !Tokens || !NumTokens)
+    return;
+
+  SmallVector<CXToken, 32> CXTokens;
+  getTokens(CXXUnit, R, CXTokens);
+
+  if (CXTokens.empty())
+    return;
+
+  *Tokens = static_cast<CXToken *>(
+      llvm::safe_malloc(sizeof(CXToken) * CXTokens.size()));
+  memmove(*Tokens, CXTokens.data(), sizeof(CXToken) * CXTokens.size());
+  *NumTokens = CXTokens.size();
+}
+
 void clang_tokenize(CXTranslationUnit TU, CXSourceRange Range, CXToken **Tokens,
                     unsigned *NumTokens) {
   LOG_FUNC_SECTION { *Log << TU << ' ' << Range; }
 
   if (Tokens)
     *Tokens = nullptr;
   if (NumTokens)
     *NumTokens = 0;
 
   if (isNotUsableTU(TU)) {
     LOG_BAD_TU(TU);
     return;
   }
 
   ASTUnit *CXXUnit = cxtu::getASTUnit(TU);
   if (!CXXUnit || !Tokens || !NumTokens)
     return;
 
   ASTUnit::ConcurrencyCheck Check(*CXXUnit);
 
   SourceRange R = cxloc::translateCXSourceRange(Range);
   if (R.isInvalid())
     return;
 
-  SmallVector<CXToken, 32> CXTokens;
-  getTokens(CXXUnit, R, CXTokens);
-
-  if (CXTokens.empty())
-    return;
-
-  *Tokens = static_cast<CXToken *>(
-      llvm::safe_malloc(sizeof(CXToken) * CXTokens.size()));
-  memmove(*Tokens, CXTokens.data(), sizeof(CXToken) * CXTokens.size());
-  *NumTokens = CXTokens.size();
+  clang_tokenize_impl(CXXUnit, R, Tokens, NumTokens);
 }
 
 void clang_disposeTokens(CXTranslationUnit TU, CXToken *Tokens,
                          unsigned NumTokens) {
   free(Tokens);
 }
 
 //===----------------------------------------------------------------------===//

clang/tools/libclang/CXSourceLocation.h

static inline SourceLocation translateSourceLocation(CXSourceLocation L) {
   return SourceLocation::getFromRawEncoding(L.int_data);
 }
 
 static inline SourceRange translateCXSourceRange(CXSourceRange R) {
   return SourceRange(SourceLocation::getFromRawEncoding(R.begin_int_data),
                      SourceLocation::getFromRawEncoding(R.end_int_data));
 }
 
-
+/// Translates CXSourceRange to SourceRange.
+/// The semantics of \p R are:
+/// R.begin_int_data is begin of the first token of the range.
+/// R.end_int_data is begin of the last token of the range.
+SourceRange translateCharRangeToTokenRange(CXTranslationUnit TU,
+                                           CXSourceRange R);
 }} // end namespace: clang::cxloc
 
 #endif