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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
