<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Hi,<div>thanks for addressing these concerns and for your contribution to LLDB.</div><div>Please find replies inlined.</div><div><br><div><div>
<div style="color: rgb(0, 0, 0); letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div style="color: rgb(0, 0, 0); letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div style="font-size: medium; orphans: 2; widows: 2; border-collapse: separate; border-spacing: 0px;"><span style="font-size: 12px; orphans: auto; widows: auto;">Enrico Granata</span><br style="font-size: 12px; orphans: auto; widows: auto;"><span style="font-size: 12px; orphans: auto; widows: auto;">📩 egranata@</span><font color="#ff2600" style="font-size: 12px; orphans: auto; widows: auto;"></font><span style="font-size: 12px; orphans: auto; widows: auto;">.com</span><br style="font-size: 12px; orphans: auto; widows: auto;"><span style="font-size: 12px; orphans: auto; widows: auto;">☎️ 27683</span></div></div></div>
</div>
<br><div><div>On May 16, 2013, at 1:13 PM, Yacine Belkadi <<a href="mailto:yacine.belkadi.1@gmail.com">yacine.belkadi.1@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">Hi Enrico,<br><br>On 05/15/2013 11:32 PM, Enrico Granata wrote:<br><blockquote type="cite">Yacine,<br><br>In addition to what Greg said, I am also slightly confused by your<br>FormatManager changes.<br>Two points here:<br><br>1) a regular expression is not really necessary. If I understand the scope<br>of the GCC bug correctly, all we need to do is match the two *exact* strings<br>std::vector<std::allocator<bool>> and std::vector<bool,<br>std::allocator<bool>, bool, std::allocator<bool> ><br>to catch all cases<br>Regexp matching is slower and it is easy to write a regexp that over-reaches<br>and matches more types than we would like to<br>If this is a workaround for a bug rather than a necessity, I would rather<br>much keep it as confined as possible, so I would definitely rework in terms<br>of exact matching<br></blockquote><br>Adding an exact string to match "std::vector<bool, std::allocator<bool>,<br>bool, std::allocator<bool> >" would indeed serve as a workaround, but only<br>for the specific case of vector<bool> + the GCC version with the bug +<br>"frame variable". It will not be enough to match the "std::vector<bool,<br>std:allocator<bool> >" that GCC initially intended to produce for<br>"std::vector<bool>”. </div></blockquote><div><br></div><div>We can have exact matches for all of these things:</div><div>- the original thing that LLDB matches</div><div>- the buggy GCC thing</div><div>- the correct GCC thing</div><div><br></div><div>Then we know exactly what we are matching and why.</div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">It will not avoid problems with the erroneous parsing<br>of other types that have templates parameters that will get duplicated. So I<br>think the first patch was important to have a complete workaround.<br><br></div></blockquote><div><br></div><div>What I meant was more along the lines of “we could erroneously match other versions of std::vector<> that should not match”.</div><div><br></div><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">As I needed to match two similar types:<br>"std::vector<bool, std:allocator<bool> >" (produced by GCC) and<br>"std::vector<std:allocator<bool> >" (produced by Clang), I thought it was<br>more appropriate to factor the two into the regex<br>"^std::vector<(bool,( )?)?std::allocator<bool> >$”.<br></div></blockquote><div><br></div>I personally tend to avoid regexes unless you really need to match a broad and potentially unbounded range of types.</div><div>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.</div><div>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 <b>small</b> number of exact matches (all of which I can actually anticipate) - <b>small</b> being key here, between 50 new exact entries and a well tested regex, definitely go for the regex!</div><div><br></div><div><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite">2) It looks like you are removing the summary for a vector of bools<br>entirely. Why?<br>You would just need to add the same summary with a new typename to the<br>category, instead your patch at lines 42 and 43 is removing the creation of<br>the summary for std::vector<bool> and I can’t see the replacement<br></blockquote><br>I removed it because it looked redundant, as it's already included in the<br>regex summary for other vectors, on line 572.<br><br></div></blockquote><div><br></div><div>Fair. I still think we should have exact matches whenever possible, but not a big deal.</div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite">This is mostly a minor cosmetic issue, and it’s not even enforced<br>consistently in the code, but I tend to add new formatters using<br>Add/Formatter /calls instead of directly playing with the FormatNavigator<br>objects<br>It’s kind of laying groundwork for potentially reworking the way built-in<br>formatters are added to an easier to maintain coding style. I have a couple<br>ideas there, just not enough time to code them :-)<br></blockquote><br>Oh yes, I should have used that. It's a lot cleaner.<br><br>Thanks for you review.<br>Yacine<br><br><blockquote type="cite">Thanks for clarifying these data points.<br><br>Enrico Granata<br>📩 egranata@.com<br>☎️ 27683</blockquote></div></blockquote></div><br></div></div></body></html>