amccarth added a comment. I have a couple inline questions. After that, it looks fine.
================ Comment at: lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:61 + for (auto it = parsed.begin(), end = parsed.end(); it != end; ++it) { // Emplace valid dependent subtrees to make target assignment independent ---------------- I would recommend making `parsed` a `const` vector. The lambda captures the iterator, and while the code currently looks fine, I'd hate for something to change in the future that could invalidate the captured iterator. ================ Comment at: lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:69 + return pair.second; + } + ---------------- This looks O(n^2). Will that matter here or will the expressions always be short? ================ Comment at: lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:84 // found target assignment program - no need to parse further - return rvalue_ast; + return it->second; } ---------------- It's a shame the `pair` loses the more descriptive field names, but I don't see a good way to make it better, so this is a useless comment. ================ Comment at: lldb/trunk/source/Symbol/PostfixExpression.cpp:101 + if (!rhs) + return {}; + result.emplace_back(lhs, rhs); ---------------- Is it correct to return an empty vector here rather than `result`? If there's one bad expression, you'll consider the entire FPO program empty. That's sounds plausible, but I thought I'd ask. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66634/new/ https://reviews.llvm.org/D66634 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits