This is an archive of the discontinued LLVM Phabricator instance.

Differential D86990

[libclang] Source range conversion
ClosedPublic

Authored by jkorous on Sep 1 2020, 5:04 PM.

Details

Reviewers
arphaman
akyrtzi
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

Diff Detail

Event Timeline

jkorous created this revision.Sep 1 2020, 5:04 PM
Herald added a subscriber: dexonsmith. · View Herald TranscriptSep 1 2020, 5:04 PM
jkorous requested review of this revision.Sep 1 2020, 5:04 PM
jkorous mentioned this in D86840: [WIP] Fix Rewriter.Sep 1 2020, 5:10 PM
akyrtzi added a comment.Sep 1 2020, 5:29 PM

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 added a comment.Sep 2 2020, 9:57 AM

@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 added a comment.Sep 2 2020, 10:08 AM
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 added a comment.Sep 2 2020, 11:10 AM

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!

jkorous updated this revision to Diff 289553.Sep 2 2020, 1:15 PM
jkorous added a child revision: D86992: [libclang] Expose Rewriter in libclang API.
akyrtzi added inline comments.Sep 2 2020, 2:20 PM
clang/tools/libclang/CXSourceLocation.h
74–78

"SourceRange" -> "CharSourceRange"

78

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

jkorous updated this revision to Diff 289839.Sep 3 2020, 5:26 PM
  • fix the doc comment
  • rename
akyrtzi accepted this revision.Sep 3 2020, 5:45 PM
This revision is now accepted and ready to land.Sep 3 2020, 5:45 PM
jkorous mentioned this in rGbaf3c77bd9f6: [libclang] Add translateCXRangeToCharRange conversion.Sep 4 2020, 9:42 AM
jkorous closed this revision.Sep 4 2020, 10:56 AM

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 <jkorous@apple.com>
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

Revision Contents

PathSize
clang/
tools/
libclang/
6 lines
6 lines

Diff 289839

clang/tools/libclang/CIndex.cpp

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 158 Lines • ▼ Show 20 Lines if (IsTokenRange && EndLoc.isValid()) {
EndLoc = EndLoc.getLocWithOffset(Length); EndLoc = EndLoc.getLocWithOffset(Length);
} }
CXSourceRange Result = { CXSourceRange Result = {
{&SM, &LangOpts}, R.getBegin().getRawEncoding(), EndLoc.getRawEncoding()}; {&SM, &LangOpts}, R.getBegin().getRawEncoding(), EndLoc.getRawEncoding()};
return Result; return Result;
} }
CharSourceRange cxloc::translateCXRangeToCharRange(CXSourceRange R) {
return CharSourceRange::getCharRange(
SourceLocation::getFromRawEncoding(R.begin_int_data),
SourceLocation::getFromRawEncoding(R.end_int_data));
}
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
// Cursor visitor. // Cursor visitor.
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
static SourceRange getRawCursorExtent(CXCursor C); static SourceRange getRawCursorExtent(CXCursor C);
static SourceRange getFullCursorExtent(CXCursor C, SourceManager &SrcMgr); static SourceRange getFullCursorExtent(CXCursor C, SourceManager &SrcMgr);
RangeComparisonResult CursorVisitor::CompareRegionOfInterest(SourceRange R) { RangeComparisonResult CursorVisitor::CompareRegionOfInterest(SourceRange R) {
▲ Show 20 Lines • Show All 8,937 Lines • Show Last 20 Lines

clang/tools/libclang/CXSourceLocation.h

Show First 20 Lines • Show All 65 Lines • ▼ Show 20 Linesstatic inline SourceLocation translateSourceLocation(CXSourceLocation L) {
return SourceLocation::getFromRawEncoding(L.int_data); return SourceLocation::getFromRawEncoding(L.int_data);
} }
static inline SourceRange translateCXSourceRange(CXSourceRange R) { static inline SourceRange translateCXSourceRange(CXSourceRange R) {
return SourceRange(SourceLocation::getFromRawEncoding(R.begin_int_data), return SourceRange(SourceLocation::getFromRawEncoding(R.begin_int_data),
SourceLocation::getFromRawEncoding(R.end_int_data)); SourceLocation::getFromRawEncoding(R.end_int_data));
} }
/// Translates CXSourceRange to CharSourceRange.
/// The semantics of \p R are:
/// R.begin_int_data is first character of the range.
/// R.end_int_data is one character past the end of the range.
CharSourceRange translateCXRangeToCharRange(CXSourceRange R);
akyrtziUnsubmitted
Not Done
ReplyInline Actions

"SourceRange" -> "CharSourceRange"

akyrtzi: "SourceRange" -> "CharSourceRange"
akyrtziUnsubmitted
Not Done
ReplyInline Actions

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

akyrtzi: Minor nitpick but maybe rename to "translateCXRangeToCharRange", to make the name more clear?
}} // end namespace: clang::cxloc }} // end namespace: clang::cxloc
#endif #endif