[llvm-branch-commits] [clang] [Serialization] Code cleanups and polish 83233 (PR #83237)

[Chuanqi Xu via llvm-branch-commits]

================
@@ -257,22 +241,12 @@ namespace clang {
       // If we have any lazy specializations, and the external AST source is
       // our chained AST reader, we can just write out the DeclIDs. Otherwise,
       // we need to resolve them to actual declarations.
-      if (Writer.Chain != Writer.Context->getExternalSource() &&
-          Common->LazySpecializations) {
+      if (Writer.Chain != Writer.Context->getExternalSource() && Writer.Chain &&
+          Writer.Chain->getLoadedSpecializationsLookupTables(D)) {
         D->LoadLazySpecializations();
-        assert(!Common->LazySpecializations);
+        assert(!Writer.Chain->getLoadedSpecializationsLookupTables(D));
       }
----------------
ChuanqiXu9 wrote:

During the process of rewriting, I just realized that this may be a significant change between D41416 and my original patch. In my original patch, I feel this is redundant since I think https://github.com/llvm/llvm-project/pull/83233/files#diff-6fe53759e8d797c328c73ada5f3324c6248a8634ef36131c7eb2b9d89192bb64R4082-R4084 can handle the specialization. But this is not true. The process of loading specializations has a strong side effect that it may trigger  more deserialization and probably loading other specializations. However, the process of re-inserting the OnDiskHashTable won't trigger deserialization. It simply moves stored hash value and the DeclID.

I guess this may be the problem why my original patch can't pass the workloads in google and ROOT. If it is the case, it shows that we lack in tree test cases for that.

https://github.com/llvm/llvm-project/pull/83237