labath added a comment. 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? ================ Comment at: lldb/unittests/SymbolFile/DWARF/DWARFASTParserClangTests.cpp:48 ASSERT_EQ(2u, die_list.size()); - ASSERT_EQ(die2, die_list[0]); - ASSERT_EQ(die3, die_list[1]); + ASSERT_NE(die_list.end(), std::find(die_list.begin(), die_list.end(), die2)); + ASSERT_NE(die_list.end(), std::find(die_list.begin(), die_list.end(), die3)); ---------------- guiandrade wrote: > I think we should not expect any particular order, right? Probably not. A more succinct way of expressing that would be `EXPECT_THAT(die_list, testing::UnorderedElementsAre(die2, die3))` 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
