[PATCH] D50740: [SourceManager] isPointWithin: avoid using isBeforeInTranslationUnit, compare buffer offsets directly for lexical correctness

2018-08-17 Thread Alex Lorenz via Phabricator via cfe-commits
arphaman added a comment. Hmm, after more analysis I realized that this is not the right solution for the rename problem. It would be correct to map one file:line:column location to a set of `SourceLocation`s, and to use the old `isPointWithin` on a set of such locations. I opened https://revie

[PATCH] D50740: [SourceManager] isPointWithin: avoid using isBeforeInTranslationUnit, compare buffer offsets directly for lexical correctness

2018-08-16 Thread Richard Smith - zygoloid via Phabricator via cfe-commits
rsmith added a comment. In https://reviews.llvm.org/D50740#1203126, @arphaman wrote: > In https://reviews.llvm.org/D50740#1202248, @jkorous wrote: > > > Hi Alex, nice work! > > > > I am just wondering if it would be beneficial to assert Loc and End are in > > the same file. It might help to catc

[PATCH] D50740: [SourceManager] isPointWithin: avoid using isBeforeInTranslationUnit, compare buffer offsets directly for lexical correctness

2018-08-16 Thread Alex Lorenz via Phabricator via cfe-commits
arphaman updated this revision to Diff 161132. arphaman marked an inline comment as done. arphaman added a comment. - Use lambda - Work with spelling locs Repository: rC Clang https://reviews.llvm.org/D50740 Files: include/clang/Basic/SourceManager.h lib/Basic/SourceManager.cpp unittes

[PATCH] D50740: [SourceManager] isPointWithin: avoid using isBeforeInTranslationUnit, compare buffer offsets directly for lexical correctness

2018-08-16 Thread Alex Lorenz via Phabricator via cfe-commits
arphaman marked 2 inline comments as done. arphaman added inline comments. Comment at: lib/Basic/SourceManager.cpp:2035 + "Passed invalid source location!"); + assert(Start.isFileID() && End.isFileID() && Loc.isFileID() && + "Passed non-file source location!"); -

[PATCH] D50740: [SourceManager] isPointWithin: avoid using isBeforeInTranslationUnit, compare buffer offsets directly for lexical correctness

2018-08-16 Thread Alex Lorenz via Phabricator via cfe-commits
arphaman added a comment. In https://reviews.llvm.org/D50740#1202248, @jkorous wrote: > Hi Alex, nice work! > > I am just wondering if it would be beneficial to assert Loc and End are in > the same file. It might help to catch bugs. I don't see the value in that unless I'm misunderstanding som

[PATCH] D50740: [SourceManager] isPointWithin: avoid using isBeforeInTranslationUnit, compare buffer offsets directly for lexical correctness

2018-08-16 Thread Eric Liu via Phabricator via cfe-commits
ioeric added inline comments. Comment at: lib/Basic/SourceManager.cpp:2035 + "Passed invalid source location!"); + assert(Start.isFileID() && End.isFileID() && Loc.isFileID() && + "Passed non-file source location!"); Why do we disallow locations

[PATCH] D50740: [SourceManager] isPointWithin: avoid using isBeforeInTranslationUnit, compare buffer offsets directly for lexical correctness

2018-08-16 Thread Jan Korous via Phabricator via cfe-commits
jkorous added a comment. Hi Alex, nice work! I am just wondering if it would be beneficial to assert Loc and End are in the same file. It might help to catch bugs. I also stumbled upon this function but not sure it makes things significantly cleaner here: https://clang.llvm.org/doxygen/SourceL

[PATCH] D50740: [SourceManager] isPointWithin: avoid using isBeforeInTranslationUnit, compare buffer offsets directly for lexical correctness

2018-08-14 Thread Alex Lorenz via Phabricator via cfe-commits
arphaman created this revision. arphaman added reviewers: jkorous, klimek, ioeric, vsapsai, ilya-biryukov. Herald added a subscriber: dexonsmith. The current implementation of `isPointWithin` uses `isBeforeInTranslationUnit` to check if a location is within a range. The problem is that `isPointWi