[PATCH] D140758: [llvm][AsmPrinter][NFC] Cleanup `GCMetadataPrinters` field

[Markus Böck via Phabricator via llvm-commits]

zero9178 marked an inline comment as done.
zero9178 added a comment.

In D140758#4019412 <https://reviews.llvm.org/D140758#4019412>, @dblaikie wrote:

> Sounds good - if you've got any links to the previous conversation that mentions the justification of using void*, might be good to link to those references in the commit message for completeness.

I thought I had found a more elaborate discussion back when I actually wrote this change, but right now I can only find the original commit that explicitly did that change (https://github.com/llvm/llvm-project/commit/2cf5f9ec05d62dac79c0df240e0b847d3658b084). 
Not explicitly said, but its the only intention I can infer from the commit description



================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:3822-3834
+  if (auto GCPI = GCMetadataPrinters.find(&S); GCPI != GCMetadataPrinters.end())
     return GCPI->second.get();
 
   auto Name = S.getName();
 
   for (const GCMetadataPrinterRegistry::entry &GCMetaPrinter :
        GCMetadataPrinterRegistry::entries())
----------------
dblaikie wrote:
> This could be modified (in a separate change) to avoid doing two lookups (one find, one insert) up GCMetadataPrinters in the cache miss case by doing something like this:
> ```
> auto [GCPI, Inserted] = GCMetadataPrinters.insert({&S, nullptr});
> if (!Inserted)
>   return GCPI->second.get();
> auto Name = S.getName();
> for (...) {
>   if (...) {
>     ...
>     GCPI->second = std::move(GMP);
>     return GCPI->second.get();
>   }
> }
> ```
> (assuming none of the operations (like `GCMetaPrinter.instantiate()`) cause other modifications to the map that would invalidate the iterator between the current `find` and `insert` calls)
Thought it was a minor change so updated the diff to include it as well


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140758/new/

https://reviews.llvm.org/D140758