labath added a comment.

In D77287#1960390 <https://reviews.llvm.org/D77287#1960390>, @compnerd wrote:

> I think that the basic test is sufficient for this.


That test does not seem to be exercising the "unload" part of this patch. It 
would also be nice to run some basic command like "image list" to verify that 
the loaded binary is indeed listed.

(I don't really have a hard objection to this being a lit test, but it does 
sound to me like at that point, this will be reimplementing TestLoadUnload.py)

> I think that the path sanitizing and conversion should be a subsequent change.

Why is that? The need for sanitation is a direct consequence of how you've 
chosen to implement this patch -- the posix version of this function does not 
do sanitation, but it does not need to, as it does not embed the library name 
into the compiled expression. I can see how the support for wide strings might 
be considered a separate feature, but given that supporting that is a matter of 
adding a single `L` to the compiled expression, I don't see a problem with 
including that here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77287/new/

https://reviews.llvm.org/D77287



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to