[PATCH] D32648: Remove line and file from DINamespace

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:1389
 
-    IsDistinct = Record[0] & 1;
+    // Upgrade: make anonymous namespaces distinct.
+    auto *Name = getMDString(Record[3]);
----------------
This is no longer accurate (& the code below's no longer correct)?


================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:1647
   Record.push_back(VE.getMetadataOrNullID(N->getScope()));
-  Record.push_back(VE.getMetadataOrNullID(N->getFile()));
+  Record.push_back(0); // Deprecated: File.
   Record.push_back(VE.getMetadataOrNullID(N->getRawName()));
----------------
So this won't improve the bitcode size? (well, it won't produce the file name/line number, but the fields will still be there, with teh minimal 0 encoding) Is that necessary? I thought in other places the number of fields in a record were detected and used to do some of the back/forwards compat?


================
Comment at: lib/IR/AsmWriter.cpp:1759
   Printer.printMetadata("scope", N->getRawScope(), /* ShouldSkipNull */ false);
   Printer.printMetadata("file", N->getRawFile());
   Printer.printBool("exportSymbols", N->getExportSymbols(), false);
----------------
Why is the file printing still here, but the line printing is removed? Seems inconsistent/surprising?


================
Comment at: lib/IR/DIBuilder.cpp:733-735
+  // It is okay to *not* make anonymous top-level namespaces distinct, because
+  // all possible children of an anonymous namespace are guaranteed to be
+  // unique.
----------------
I'd probably ramble on a bit more here - explaining that not only is it OK, but an explicit tradeoff of link time versus memory usage versus code simplicity & could be revisited if those tradeoffs turn out to be not right.

Also, from my perspective it's not so much because the children (& maybe better to rephrase that "the things referring to an anonymous namespace" because the namespace doesn't have children in the mechanical sense - other nodes refer to it... eh anyway, whatever you think's best) are unique but because they're necessarily in different CUs & don't move between them because of how they're referenced.


https://reviews.llvm.org/D32648