[Lldb-commits] [PATCH] D70393: [lldb] Fix NSURL data formatter truncation issue in Swift

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

In D70393#1751614 <https://reviews.llvm.org/D70393#1751614>, @poya wrote:

> In D70393#1751514 <https://reviews.llvm.org/D70393#1751514>, @teemperor wrote:
>
> > But having a test for this upstream is what should ideally happen instead of just testing this downstream.
>
>
> There is already a test for the NSURL data formatter upstream for the ObjC context, but the same summary provider is also used with Swift and it was only there it was being incorrectly truncated, because assumption of prefix length. So if I understood @davide correctly he wanted a test added in the Swift fork for the Swift case.


Usually code should not just be tested downstream but also here (which is of course tricky with Swift-specific stuff as Swift is only downstream). So if you see a way to test this without just testing it in swift-lldb that would be great, but on the other hand this is just touching ObjC formatters which are anyway Apple-specific code, so it's not the end of the world if not...



================
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:
> 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.


================
Comment at: lldb/source/Plugins/Language/ObjC/Cocoa.cpp:673
+  if (!first_str.empty())
+    first_str = first_str.drop_back();
+
----------------
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).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70393/new/

https://reviews.llvm.org/D70393