<br><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Sep 17, 2012 at 1:25 PM, Gábor Horváth <span dir="ltr"><<a href="mailto:xazax.hun@gmail.com" target="_blank" class="cremed">xazax.hun@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi!<br><br>Thanks for your review. I readjusted my diff, and sliced it to 4 parts (however I did it manually, so there may be problems when one wants to apply them automatically, if there is a better way to slice diffs, please let me know). <br>
</blockquote><div><br></div><div>As discussed on IRC, I think using git (git-svn) is a very good option to juggle multiple LLVM/Clang changes. </div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The expr()s are necessary, because those matchers are implemented via VariadicDynCastAllOfMatcher, where the SourceT is Expr. I implemented it that way, because I mimiced the existing implementations, for example the reinterpret_cast matcher (probably it was implemented that way to increase type safety?). <br>
</blockquote><div><br></div><div>I have just changed that everywhere else. It was there for historic reasons and does not really provide additional type-safety.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I will explore <a href="http://llvm-reviews.chandlerc.com/" target="_blank" class="cremed">http://llvm-reviews.chandlerc.com/</a> later, thanks for letting me know.<span class="HOEnZb"><font color="#888888"><br><br>Gábor</font></span></blockquote>
<div><br></div><div>Cheers,</div><div>Daniel</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><br><br><div class="gmail_quote">
On 17 September 2012 08:38, Daniel Jasper <span dir="ltr"><<a href="mailto:djasper@google.com" target="_blank" class="cremed">djasper@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Gábor,<div><br></div><div>thanks for working on this. In general, although everything in your patch are "matchers", I'd prefer if you can make multiple smaller patches out of it. In this cases, I would like to have 4 patches:</div>
<div> 1. cStyleCastExpr (already discussed, no-brainer)</div><div> 2. C++11-specific matchers (so we can decide whether the patch includes everything to support them)</div><div> 3. Matchers where something seems off ("not yet supported?")</div>
<div> 4. The remaining trivial matchers (breakStmt, continueStmt, ...)</div><div><br></div><div>As for the patch itself:</div><div><div><br></div><div> testing::AssertionResult matchesConditionally(const std::string &Code,</div>
<div> const T &AMatcher,</div><div> - bool ExpectMatch) {</div><div> + bool ExpectMatch,</div>
<div> +<span style="white-space:pre-wrap"> </span> bool CXX11 = false) {</div><div><br></div></div><div>The indent is off. Also, add the default value for the new parameter to the two existing call sites and not as a parameter default. There is just one test case that fails with -std=c++11, so we should probably make that the default language for matches() and notMatches().</div>
<div><br></div><div>Also, I find methods with multiple bool parameters confusing as I can never remember which one means which. As it is somewhat likely that we will want to support more language variants, I would turn this parameter either into an enum or into a string.</div>
<div><br></div><div><div> +TEST(Matcher, Lambda) {</div><div> + EXPECT_TRUE(matchesConditionally("auto f = [&] (int i) { return i; };",</div><div> + lambdaExpr(), true, true));</div><div>
+</div></div><div><br></div><div><span style="color:rgb(51,51,51);font-family:sans-serif;font-size:13px">If you break function call arguments onto multiple lines, align the subsequent lines with the first argument (here 'lambdaExpr' right under '"auto..'). Also at all other places in your patch.</span><br>
</div><div><span style="color:rgb(51,51,51);font-family:sans-serif;font-size:13px"><br></span></div><div><span style="color:rgb(51,51,51);font-family:sans-serif;font-size:13px"><div> +TEST(CStyleCast, MatchesSimpleCase) {</div>
<div> + EXPECT_TRUE(matches("char* p = (char*)(&p);",</div><div> + expr(cStyleCastExpr())));</div><div> +}</div></span></div><div><span style="color:rgb(51,51,51);font-family:sans-serif;font-size:13px"><br>
</span></div><div>This does not seem like a very simple case to me ;-). How about something like "int i = (int) 2.2f;"? I don't think you need the 'expr()' around the 'cStyleCastExpr()' (also at most other places).</div>
<div><br></div><div><div> +TEST(CStyleCast, DoesNotMatchOtherCasts) {</div><div> + EXPECT_TRUE(notMatches("char* p = static_cast<char*>(0);",</div><div> + expr(cStyleCastExpr())));</div>
<div> + ..</div></div><div><br></div><div>Putting all the cases into one code snippet and the have cStyleCastExpr() not match on it might be slightly easier to read.</div><div><br></div><div>I'll take another look after you have split up the patches, but I think we can get most of them in quite quickly. Note that we are currently testing a code review system on <a href="http://llvm-reviews.chandlerc.com/" target="_blank" class="cremed">http://llvm-reviews.chandlerc.com/</a>. I personally find it easier to review patches there, so feel free to use it (but it is of course entirely optional).</div>
<div><br></div><div>Cheers,<br></div><div>Daniel</div><div><div>
<div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Sep 14, 2012 at 10:16 PM, Gábor Horváth <span dir="ltr"><<a href="mailto:xazax.hun@gmail.com" target="_blank" class="cremed">xazax.hun@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Greetings!<br><br>I am interested in participating in developing the tooling library. To get started I tried to reach the low hanging fruits, and added some matchers that were missing, I hope at least some of them will be pushed to upstream. I also included a some C++11 node matchers. <br>
<br>The diff is attached. Any feedback is welcome.<br><br>Thanks,<br>Gabor<br>
</blockquote></div><br></div>
</div></div></blockquote></div><br>
</div></div></blockquote></div><br></div>