bruno marked 2 inline comments as done. bruno added a comment.
> Nice! Are you planning to address the FIXME's in a later update of this patch? The FIXME's are just replaying what the code around does, both error dropping and `FileEntryRef` are recent changes that didn't make all the way through yet. I should help improving that at some point cause it will overall be good for modules, but those changes are orthogonal to this patch. ================ Comment at: clang/lib/Serialization/ASTWriter.cpp:1767 bool IsTopLevelModuleMap; + uint32_t ContentHash[2]; }; ---------------- aprantl wrote: > Why is this not a uint64_t? Serializing a `uint64_t` directly instead of two `uint32_t` gives me a slightly bigger final cache. One could argue that the value is negligible (+800 bytes for a 40MB cache), but here's the rationale :) ================ Comment at: clang/test/Modules/validate-file-content.m:14 +// RUN: %clang_cc1 -emit-pch -fmodules-cache-path=%t/cache -fmodules -fimplicit-module-maps -o %t/a.pch -I %t -x objective-c-header %t/a.h -fvalidate-ast-input-files-content +// RUN: touch -m -a -A 01 %t/m.h +// RUN: %clang_cc1 -fsyntax-only -fmodules-cache-path=%t/cache -fmodules -fimplicit-module-maps -I %t -include-pch %t/a.pch %s -verify -fvalidate-ast-input-files-content ---------------- aprantl wrote: > Are there other tests that use touch and file timestamps in general? I wonder > if this could cause random issues when building on NFS or filesystems with a > really low timestamp granularity. I don't have a better suggestion either > since the mtime is part of the code being tested here. > Are there other tests that use touch and file timestamps in general? Yes, I found quite a few around the tests. However, I just tested `touch -m -a -A 01` on linux and it seems that `-A` isn't supported, maybe I'll just change the entire mtime for some time in the future, that's likely more portable. > I wonder if this could cause random issues when building on NFS or > filesystems with a really low timestamp granularity. I don't have a better > suggestion either since the mtime is part of the code being tested here. Right, perhaps changing the mtime completely might help here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67249/new/ https://reviews.llvm.org/D67249 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
