Hi,
thanks for addressing these concerns and for your contribution to LLDB.
Please find replies inlined.
Enrico Granata
egranata@.com
27683
Hi Enrico,
Yacine,
In addition to what Greg said, I am also slightly confused by your
FormatManager changes.
Two points here:
- 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> and std::vector<bool,
std::allocator, bool, std::allocator >
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, std::allocator >” would indeed serve as a workaround, but only
for the specific case of vector + the GCC version with the bug +
“frame variable”. It will not be enough to match the “std::vector<bool,
std:allocator >” that GCC initially intended to produce for
"std::vector”.
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 >” (produced by GCC) and
“std::vector<std:allocator >” (produced by Clang), I thought it was
more appropriate to factor the two into the regex
"^std::vector<(bool,( )?)?std::allocator >$”.
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 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!
- 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 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.
