[Lldb-commits] [PATCH] D66634: Postfix: move more code out of the PDB plugin

Adrian McCarthy via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Mon Aug 26 08:27:43 PDT 2019

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.




More information about the lldb-commits mailing list