dexonsmith added a comment. In D136624#3900593 <https://reviews.llvm.org/D136624#3900593>, @jansvoboda11 wrote:
> Ah, I forgot to mention this. Building the modules is now only 0.2% slower > and importing them 1.2% faster (compared to PCMs with all input files > serialized). Awesome. All upside then :). I added a few nitpick-y comments inline. I can take another look once you've made the PCMs bit-for-bit identical and updated the test (is that happening here, or in a separate review)? ================ Comment at: clang/include/clang/Basic/SourceManager.h:1831-1833 + bool isLoadedOffset(SourceLocation::UIntTy SLocOffset) const { + return SLocOffset >= CurrentLoadedOffset; + } ---------------- jansvoboda11 wrote: > dexonsmith wrote: > > The logic for `isLoadedOffset()` suggests that it could maybe be subsumed > > with "location past the end"? > I don't think so - we don't want to adjust loaded offsets. Their invariant is > that they grow from 2^31 downwards. > > We do want to adjust local offsets past the last non-affecting file though. Oops, right! ================ Comment at: clang/lib/Serialization/ASTWriter.cpp:4538 + for (unsigned I = 1; I != N; ++I) { + // Get this source location entry. + const SrcMgr::SLocEntry *SLoc = &SrcMgr.getLocalSLocEntry(I); ---------------- Not sure this comment adds much on top of the code on the next line. A sentence before the `for` loop describing the overall approach might be useful though. ================ Comment at: clang/lib/Serialization/ASTWriter.cpp:4550-4554 + if (isModuleMap(File.getFileCharacteristic()) && + !isSystem(File.getFileCharacteristic()) && + !AffectingModuleMaps.empty() && + AffectingModuleMaps.find(Cache->OrigEntry) == + AffectingModuleMaps.end()) { ---------------- You can reduce nesting by inverting this condition and `continue`. ================ Comment at: clang/lib/Serialization/ASTWriter.cpp:4562-4563 + NonAffectingFileIDs.back().ID == FID.ID - 1) { + // If the previous file was non-affecting as well, just extend its entry + // with our information. + NonAffectingFileIDs.back() = FID; ---------------- I'd slightly prefer the comment *before* the `if`, due to how folding tends to work in editors (see the comment even when the code is folded). Probably relies on dropping the `else` (see below). ================ Comment at: clang/lib/Serialization/ASTWriter.cpp:4568 + NonAffectingFileIDsAdjustments.back() = Count; + } else { + NonAffectingFileIDs.push_back(FID); ---------------- I suggest `continue` before the `else` to avoid adding nesting for insertion. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136624/new/ https://reviews.llvm.org/D136624 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits