[PATCH] D148975: -fdebug-prefix-map=: make the last win when multiple prefixes match

[Fangrui Song via Phabricator via cfe-commits]

MaskRay marked an inline comment as done.
MaskRay added inline comments.


================
Comment at: clang/include/clang/Basic/CodeGenOptions.h:209
 
-  std::map<std::string, std::string> DebugPrefixMap;
+  llvm::SmallVector<std::pair<std::string, std::string>, 0> DebugPrefixMap;
   std::map<std::string, std::string> CoveragePrefixMap;
----------------
scott.linder wrote:
> MaskRay wrote:
> > scott.linder wrote:
> > > What benefit does forcing allocation have? Why not use the default, or switch to `std::vector`?
> > This doesn't force allocation. I use inline element size 0 to make the member variable smaller than a `std::vector`.
> > `std::vector` likely compiles to larger code.
> Sorry, I just didn't realize this was idiomatic (i.e. I hadn't read https://llvm.org/docs/ProgrammersManual.html#vector), and I didn't see other similar uses in the file.
> 
> There are 15 `std::vector` members of `CodeGenOptions`, but no `SmallVector<_, 0>` members; maybe the rest can be converted in an NFC patch, so things are consistent?
It seems that SmallVector is used more frequently but std::vector is used as well. I'd prefer that we don't change the types for existing variables...
```
% rg SmallVector lib -l | wc -l => 439
% rg std::vector lib -l | wc -l => 255
```


================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:72-79
 CGDebugInfo::CGDebugInfo(CodeGenModule &CGM)
     : CGM(CGM), DebugKind(CGM.getCodeGenOpts().getDebugInfo()),
       DebugTypeExtRefs(CGM.getCodeGenOpts().DebugTypeExtRefs),
       DBuilder(CGM.getModule()) {
-  for (const auto &KV : CGM.getCodeGenOpts().DebugPrefixMap)
-    DebugPrefixMap[KV.first] = KV.second;
+  for (const auto &[From, To] : CGM.getCodeGenOpts().DebugPrefixMap)
+    DebugPrefixMap.emplace_back(From, To);
   CreateCompileUnit();
----------------
scott.linder wrote:
> MaskRay wrote:
> > scott.linder wrote:
> > > Can you use the member-initializer-list here?
> > No, this doesn't work. We convert a vector of `std::string` to a vector of `StringRef`.
> If all we are doing is creating another vector which shares the underlying strings with the original, why not just save a reference to the original vector? Is the cost of the extra dereference when accessing it greater than the cost of traversing it plus the extra storage for the `StringRef`s?
> 
> It seems like the original utility was just to get the `std::map` sorting behavior, which we no longer need.
Thanks for the giving this more attention. Actually I found that the variable can be removed. Removed:)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148975