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