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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits