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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
