poya marked 2 inline comments as done.
poya added inline comments.
================
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:
> poya wrote:
> > 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?
> > Was trying to follow the coding style used elsewhere in the same file for
> > the helper function name
>
> Fair point.
>
> > 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?
>
> I mean it's not just concatenating first and second, but removes string
> prefix/suffix, one arbitrary character that is the a quotation character we
> don't want in summaries and then returns them with in the format for that we
> expect for summaries (`A -- B`), so it seems quite specific to expect a
> summary and summary_base.
Agree that it's not a pure concatenation function, but just to avoid any
misunderstandings I want to clarify that it retains the enclosing prefix/suffix
and quote marks in its output.
It concatenates
@"test.html" and @"http://lldb.llvm.org";
to
@"test.html -- http://lldb.llvm.org";
which is the actual summary output
I can try switching the arg names though.
================
Comment at: lldb/source/Plugins/Language/ObjC/Cocoa.cpp:673
+ if (!first_str.empty())
+ first_str = first_str.drop_back();
+
----------------
teemperor wrote:
> poya wrote:
> > 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
> If this changes we probably want to know (and we maybe should even assert
> that this happens).
Got it. As I saw that in some contexts the quote character was ' instead of "
(not here though), I was just wary about potentially introducing a similar
failure point as I was fixing, '@' string prefix in obj-c vs no prefix in
Swift, which had gone unnoticed.
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
