[PATCH] D134685: Fix SourceManager::isBeforeInTranslationUnit bug with token-pasting

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

Thanks for digging into the rabbit role and the great analysis. It looks good from my side.

Re the test case `ID(I TWO 3)` we discussed yesterday, I verified that there is no issue. (we don't merge the `1 2 3` into a single SLOCEntry, each one have a dedicated FileID, and FileID(2) < FileID(3)).

>> However I tried to avoid mixing these with subtle behavior changes, and will send a followup instead.
>
> D134694 <https://reviews.llvm.org/D134694> if you're interested. I guess I should try to get performance measures though...

While doing the review, I agree that this part of code is unnecessary complicated, I think doing cleanup is probably a good idea.



================
Comment at: clang/lib/Basic/SourceManager.cpp:2108
     // quit early. The other way round unfortunately remains suboptimal.
-  } while (LOffs.first != ROffs.first && !MoveUpIncludeHierarchy(LOffs, *this));
-  LocSet::iterator I;
-  while((I = LChain.find(ROffs.first)) == LChain.end()) {
-    if (MoveUpIncludeHierarchy(ROffs, *this))
-      break; // Met at topmost file.
-  }
-  if (I != LChain.end())
-    LOffs = *I;
+    if (LOffs.first == ROffs.first)
+      break;
----------------
nit:  I found the first/second usage really hurts the code readability here (using the struct `Entry` int all places is probably better, but this requires some changes to the existing interface, no required action here).


================
Comment at: clang/lib/Basic/SourceManager.cpp:2126
+      // This changes the relative order of local vs loaded FileIDs, but it
+      // doesn't matter as these are never mixed in macro expansion.
+      unsigned LParent = I->second.ParentFID.ID;
----------------
maybe add an assertion for "local and load FileIDs  are never mixed".


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134685/new/

https://reviews.llvm.org/D134685