poya marked an inline comment as done.
poya added a comment.
================
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:
> > 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.
Was trying to follow the coding style used elsewhere in the same file for the
helper function name
================
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) {
----------------
poya wrote:
> teemperor wrote:
> > 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.
> Was trying to follow the coding style used elsewhere in the same file for the
> helper function name
Didn't think summary and base_summary made it very simple to reason about,
perhaps destination and source, or front and back, would make more sense for a
concatenation function?
================
Comment at: lldb/source/Plugins/Language/ObjC/Cocoa.cpp:673
+ if (!first_str.empty())
+ first_str = first_str.drop_back();
+
----------------
teemperor wrote:
> This can all be just:
>
> ```
> first_str.consume_back(suffix);
> first_str.consume_back("\"");
> ```
>
> And the same thing below.
Didn't want to make an assumption of the quote character used if it ever changed
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
