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

[Adrian McCarthy via Phabricator via lldb-commits]

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