[PATCH] D136624: [clang][modules] Account for non-affecting inputs in `ASTWriter`

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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