guiandrade added a comment. In D67022#1658057 <https://reviews.llvm.org/D67022#1658057>, @clayborg wrote:
> In D67022#1656874 <https://reviews.llvm.org/D67022#1656874>, @labath wrote: > > > Thanks for the clarification Greg. > > > > Functionally, this patch seems fine, but I am still wondering if we > > shouldn't make this more efficient. std::set is not the most > > memory-efficient structure, and so creating a std::set entry just to store > > one bit seems wasteful, particularly as there is already a container which > > uses the context as a key (`m_decl_ctx_to_die`). Could just shove this bit > > into the `m_decl_ctx_to_die` map (probably by changing it to something > > functionally equivalent to `map<DeclContext, pair<bool, > > vector<DWARFDie>>>`)? Given that the sole "raison d´être" of this map is to > > enable the `ParseDeclsForContext` functionality, I don't think that having > > that flag be stored there should be awkward. Quite the opposite, it would > > remove the awkward back-and-forth between the SymbolFileDWARF and the > > DWARFASTParsers (symbol file calls `ForEachDIEInDeclContext`, passing it a > > callback; then ast parser calls the callback; finally the callback > > immediately re-enters the parser via `GetDeclForUIDFromDWARF`) -- instead > > we could just have the SymbolFileDWARF do > > `ast_parser->PleaseParseAllDiesInThisContextIfTheyHaventBeenParsedAlready(ctx)` > > and have everything happen inside the parser. > > > > So, overall, it seems to me that this way, we could improve the code > > readability while reducing the time, and avoiding a bigger memory increase. > > In fact, since we now will not be using the list of dies after they have > > been parsed once, we could even free up the vector<DWARFDie> after the > > parsing is complete, and thereby reduce the memory footprint as well. That > > would be a win-win-win scenario. :) > > > > WDYT? > > > Great idea, but this idea gave me the idea to use "m_decl_ctx_to_die" as is, > and just remove the entry once we have parse all decls. Then we free the > memory and don't need a bit. If there is no entry in the m_decl_ctx_to_die > map, then ForEachDIEInDeclContext will just not iterate at all? Thanks for the ideas, I think they greatly simplify the code. Let me know what you guys think of my implementation. I think the main issue with it is that it makes it hard to unit test it... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67022/new/ https://reviews.llvm.org/D67022 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
