[Lldb-commits] [PATCH] D134033: [lldb/Plugins] Improve error reporting when reading memory in Scripted Process

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

Cool. Thanks.

In D134033#3935424 <https://reviews.llvm.org/D134033#3935424>, @mib wrote:

> In D134033#3933936 <https://reviews.llvm.org/D134033#3933936>, @labath wrote:
>
>> I kinda like it. One thing that I think would help with the readability is if the "transformation" methods were grouped according to the type of the object being transformed, rather than according to the direction. So something like:
>>
>>   // general transform
>>   // general reverse
>>   // Status transform
>>   // Status reverse
>>
>> instead of
>>
>>   // general transform
>>   // Status transform
>>   // general reverse
>>   // Status reverse
>>
>> Also I don't think that these structs (`transformation`, `reverse_transformation`) wrapping the transformation functions are really necessary. It's true that one cannot partially specialize functions, but one of the reasons for that is this is normally not necessary -- regular function overloading <https://godbolt.org/z/114KeeK3f> can do most of that as well.
>
> Thanks for the feedback! I really appreciate it :) For now I reordered the structs as you suggested but I'm thinking of moving all the transformation related code to an anonymous namespace above the class. I'll do that in a follow-up patch :)

You can't have an anonymous namespace in a header (well.. you can, but it will break in very interesting ways). You can put it inside some `detail` or `internal` namespace, if you think it helps.



================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.h:167-168
+                              std::tuple<Us...> &transformed_args) {
+    if (sizeof...(Ts) != sizeof...(Us))
+      return false;
+
----------------
this could be a static_assert


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

https://reviews.llvm.org/D134033