dexonsmith added inline comments.
================ Comment at: clang/lib/Lex/HeaderSearch.cpp:888 bool InUserSpecifiedSystemFramework = false; - bool HasBeenMapped = false; + bool CurrentInHeaderMap = false; bool IsFrameworkFoundInDir = false; ---------------- Why not name this the same way as the parameter? (`IsInHeaderMap`) ================ Comment at: clang/lib/Lex/HeaderSearch.cpp:896-897 + assert(CurrentInHeaderMap && "MappedName should come from a header map"); CacheLookup.MappedName = - copyString(Filename, LookupFileCache.getAllocator()); - if (IsMapped) - *IsMapped = true; + copyString(MappedName, LookupFileCache.getAllocator()); + HasBeenMapped = true; ---------------- It's not obvious to me why this changed from `Filename` to `MappedName`. Can you explain it? ================ Comment at: clang/lib/Lex/HeaderSearch.cpp:898 + copyString(MappedName, LookupFileCache.getAllocator()); + HasBeenMapped = true; } ---------------- Do we need `HasBeenMapped` outside of the loop, or can you just use the loop-local variable? ================ Comment at: clang/lib/Lex/HeaderSearch.cpp:908-909 + if (IsMapped) + *IsMapped = CurrentInHeaderMap || HasBeenMapped; + ---------------- Are you intentionally delaying this until after the `if (!File)` check? If so, why? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58094/new/ https://reviews.llvm.org/D58094 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits