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

Richard Smith - zygoloid via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Aug 16 15:48:35 PDT 2018


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 catch bugs.
>
>
> I don't see the value in that unless I'm misunderstanding something. We already check if Loc and End are in the same file, and return false if they're not.


I think we should assert that `Start` and `End` are in the same file. If not, the query doesn't make any sense; the range is malformed. And maybe we should also assert that `Start` is before `End` in that file.



================
Comment at: include/clang/Basic/SourceManager.h:1585-1586
+  /// located in the same inclusion span as the range).
   bool isPointWithin(SourceLocation Location, SourceLocation Start,
-                     SourceLocation End) const {
-    return Location == Start || Location == End ||
-           (isBeforeInTranslationUnit(Start, Location) &&
-            isBeforeInTranslationUnit(Location, End));
-  }
+                     SourceLocation End) const;
 
----------------
Please rename this function to make it clearer what it's doing. The old implementation is the obvious one for a function with this name. Maybe `isSpellingLocInFileRange` or similar?


================
Comment at: lib/Basic/SourceManager.cpp:2038-2050
+  auto isOrderedInSameFile =
+      [&](SourceLocation RangeLoc,
+          llvm::function_ref<bool(unsigned, unsigned)> Comparator) {
+        std::pair<FileID, unsigned> DecomposedRangeLoc =
+            getDecomposedLoc(getSpellingLoc(RangeLoc));
+        // Ensure that we check for FileEntry equivalency to account for
+        // multiple inclusions.
----------------
Does this do the right thing if both points are within (eg) macro expansion ranges in the same file? Eg, given:

```
// A
#define FOO BAR
// B
FOO
// C
```

is the macro-expanded `BAR` token between `B` and `C`? Is it between `A` and `B`?

I would guess that the intended semantics here are:

 * `Start` and `End` are expected to be file locations in the same inclusion of the same file, with `Start` occuring no later than `End`.
 * `Loc` is an arbitrary location.
 * Function returns `true` if the spelling location of `Loc` is in the same source file as `Start` and `End`, and occurs between them; `false` otherwise.

Is that right? If so, please write that down somewhere; the current function comment is too vague. (And if not, please write down what the intended rule is.)


Repository:
  rC Clang

https://reviews.llvm.org/D50740





More information about the cfe-commits mailing list