teemperor requested changes to this revision. teemperor added inline comments. This revision now requires changes to proceed.
================ Comment at: lldb/source/Plugins/Language/ObjC/Cocoa.cpp:662 +static void NSURL_ConcatSummary(StreamString &first, const StreamString &second, + std::string prefix, std::string suffix) { ---------------- teemperor wrote: > I am not a big fan of the "Class_method" way of extending classes. Maybe > `ConcatSummaryForNSURL`? first and second also not very descriptive variables names. What about the `summary` and `base_summary` names from where we call the method? ================ Comment at: lldb/source/Plugins/Language/ObjC/Cocoa.cpp:662 +static void NSURL_ConcatSummary(StreamString &first, const StreamString &second, + std::string prefix, std::string suffix) { ---------------- teemperor wrote: > teemperor wrote: > > I am not a big fan of the "Class_method" way of extending classes. Maybe > > `ConcatSummaryForNSURL`? > first and second also not very descriptive variables names. What about the > `summary` and `base_summary` names from where we call the method? Having `first` as both a in and out parameter makes the control flow not very trivial to understand. You could just compute a result and return that result (and then assign this to the `summary` variable in the calling context)? Also all parameters to this function can just be `llvm::StringRef` from what I can see. ================ Comment at: lldb/source/Plugins/Language/ObjC/Cocoa.cpp:673 + if (!first_str.empty()) + first_str = first_str.drop_back(); + ---------------- This can all be just: ``` first_str.consume_back(suffix); first_str.consume_back("\""); ``` And the same thing below. ================ Comment at: lldb/source/Plugins/Language/ObjC/Cocoa.cpp:684 + first.Clear(); + first.Printf("%s -- %s", first_str.str().c_str(), second_str.str().c_str()); + } ---------------- This could just be `return first_str + " -- " + second_str;` if you return instead of having an in/out parameter. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70393/new/ https://reviews.llvm.org/D70393 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits