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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
