Hi Enrico, On 05/15/2013 11:32 PM, Enrico Granata wrote: > Yacine, > > In addition to what Greg said, I am also slightly confused by your > FormatManager changes. > Two points here: > > 1) a regular expression is not really necessary. If I understand the scope > of the GCC bug correctly, all we need to do is match the two *exact* strings > std::vector<std::allocator<bool>> and std::vector<bool, > std::allocator<bool>, bool, std::allocator<bool> > > to catch all cases > Regexp matching is slower and it is easy to write a regexp that over-reaches > and matches more types than we would like to > If this is a workaround for a bug rather than a necessity, I would rather > much keep it as confined as possible, so I would definitely rework in terms > of exact matching
Adding an exact string to match "std::vector<bool, std::allocator<bool>, bool, std::allocator<bool> >" would indeed serve as a workaround, but only for the specific case of vector<bool> + the GCC version with the bug + "frame variable". It will not be enough to match the "std::vector<bool, std:allocator<bool> >" that GCC initially intended to produce for "std::vector<bool>". It will not avoid problems with the erroneous parsing of other types that have templates parameters that will get duplicated. So I think the first patch was important to have a complete workaround. As I needed to match two similar types: "std::vector<bool, std:allocator<bool> >" (produced by GCC) and "std::vector<std:allocator<bool> >" (produced by Clang), I thought it was more appropriate to factor the two into the regex "^std::vector<(bool,( )?)?std::allocator<bool> >$". > 2) It looks like you are removing the summary for a vector of bools > entirely. Why? > You would just need to add the same summary with a new typename to the > category, instead your patch at lines 42 and 43 is removing the creation of > the summary for std::vector<bool> and I can’t see the replacement I removed it because it looked redundant, as it's already included in the regex summary for other vectors, on line 572. > This is mostly a minor cosmetic issue, and it’s not even enforced > consistently in the code, but I tend to add new formatters using > Add/Formatter /calls instead of directly playing with the FormatNavigator > objects > It’s kind of laying groundwork for potentially reworking the way built-in > formatters are added to an easier to maintain coding style. I have a couple > ideas there, just not enough time to code them :-) Oh yes, I should have used that. It's a lot cleaner. Thanks for you review. Yacine > Thanks for clarifying these data points. > > Enrico Granata > 📩 egranata@.com > ☎️ 27683 _______________________________________________ lldb-dev mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
