[lldb-dev] [Bug 15301] New: LLDB prints incorrect size of libstdc++ vector<bool> containers (when inferior built with GCC on Linux)

[Enrico Granata]

Hi,
thanks for addressing these concerns and for your contribution to LLDB.
Please find replies inlined.

Enrico Granata
📩 egranata@.com
☎️ 27683

On May 16, 2013, at 1:13 PM, Yacine Belkadi <yacine.belkadi.1 at gmail.com> wrote:

> 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>”.

We can have exact matches for all of these things:
- the original thing that LLDB matches
- the buggy GCC thing
- the correct GCC thing

Then we know exactly what we are matching and why.

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

What I meant was more along the lines of “we could erroneously match other versions of std::vector<> that should not match”.

> 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> >$”.

I personally tend to avoid regexes unless you really need to match a broad and potentially unbounded range of types.
In this case, it seems we have two or three different names, and it would also be good to track that one of these is just a workaround for a bug, rather than a real incarnation of the class.
It is not critical because we do formatters caching (so we would look at the regexes once, figure out the match for vector<bool> and then reuse it until the user changes anything in the formatters) but I feel it makes things more obvious to use regex matches only when a regex is really needed (I can’t anticipate anything a user can want to put in a vector, can I?) vs. a small number of exact matches (all of which I can actually anticipate) - small being key here, between 50 new exact entries and a well tested regex, definitely go for the regex!

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

Fair. I still think we should have exact matches whenever possible, but not a big deal.

>> 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20130516/06ea79ff/attachment.html>