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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
