<div dir="ltr">Somehow cfe-commits got lost.. Adding that back to CC..<br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Oct 24, 2013 at 9:41 AM, 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"><div dir="ltr">Yes you are right, I was confused. However, my confusion was stemming from the fact that your tests don't include the interesting case:<div>
<br></div><div><div>+TEST_F(FormatTest, BreaksBeforeClassInheritanceComma) {</div>
<div>+ FormatStyle Style = getWebKitStyle();</div><div>+ Style.BreakClassInheritanceListsBeforeComma = true;</div><div>+</div><div>+ verifyFormat("namespace outer {\n"</div><div>+ "int i;\n"</div>
<div>+ "namespace inner {\n"</div><div>+ " int i;\n"</div><div>+ " class Aaaa : public Bbb\n"</div><div>+ " , protected Ccc {\n"</div>
</div><div><br></div><div>I would be interesting to see what happens if you actually have to break before "public" (i.e. because you'd otherwise violate the column limit).</div><div><br></div><div>Come to think of that, is this patch really doing what it is supposed to do? Can you specify what exactly BreakClassInheritanceListsBeforeColon is supposed to do? The options are:</div>
<div>1) Always break before the colon</div><div>2) Break before the colon if the entire line (up to "{") doesn't fit into the column limit</div><div>3) Break before the colon if the first base class (i.e. everything up to ",") does not fit on one line</div>
<div>4) Break before the colon if the first base class does not fit on one line or one of the following base classes cannot be aligned nicely</div><div><br></div><div>I would intuitively think that option #4 is what you are trying to implement (based on configuration flag's comment). Maybe we should state this more clearly and add more tests.</div>
<div><br></div><div>If you are going to write more tests, feel free to make them shorter. Leave out the namespaces and global variables. And maybe move the constructor with initializers to its own verifyFormat(). We only need to show once (for each config option) that it does not influence constructor initializers.</div>
<div><br></div><div>Cheers and sorry for not spotting this earlier!</div><span class="HOEnZb"><font color="#888888"><div>Daniel</div></font></span></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><br>
<div class="gmail_quote">On Thu, Oct 24, 2013 at 9:24 AM, Curdeius Curdeius <span dir="ltr"><<a href="mailto:curdeius@gmail.com" target="_blank" class="cremed">curdeius@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi Daniel,<div>thanks again.</div><div>Just a small remark.</div><div>You say I'm not testing all the 4 possible cases, but (in my opinion), I do.</div>
<div>I created 3 tests:</div><div><ul><li>TEST_F(FormatTest, BreaksBeforeClassInheritanceColonAndComma) - true, true case<br>
</li><li>TEST_F(FormatTest, BreaksBeforeClassInheritanceComma) - false, true case<br></li><li>TEST_F(FormatTest, BreaksBeforeClassInheritanceColon) = true, false case<br></li></ul></div><div>The 4th case (false, false) is taken into account by existing tests.</div>
<div><br><div class="gmail_extra">I'll take into account and write back ASAP.</div><div class="gmail_extra"><br></div><div class="gmail_extra">Cheers,</div><div class="gmail_extra">Marek<div><div><br><br><div class="gmail_quote">
2013/10/23 Daniel Jasper <span dir="ltr"><<a href="mailto:djasper@google.com" target="_blank" class="cremed">djasper@google.com</a>></span><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote"><div>On Wed, Oct 23, 2013 at 3:44 PM, Curdeius Curdeius <span dir="ltr"><<a href="mailto:curdeius@gmail.com" target="_blank" class="cremed">curdeius@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">Hi Daniel,<div>thanks for your feedback and sorry for the late response.</div>
<div><br></div><div>I attach the patch.</div><div><br></div><div>My comments to your remarks below.</div><div class="gmail_extra">
<br><div class="gmail_quote"><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">
<div>* How is the new test in FormatsWithWebKitStyle related?<br>
</div></div></blockquote><div><br></div></div><div>I removed the unnecessary mess.</div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div>* Either collapse both parameters into a single parameter or add test cases for all combinations.</div></div></blockquote><div><br></div></div><div>I did so.</div></div></div></div></blockquote><div><br>
</div></div><div>Not really. You are still not testing ..BeforeColon = false and ..BeforeComma = true. I agree that it does not make much sense, but then, an enum containing exactly the three configuration values seems to be a better solution. You can take a look at UseTabStyle for an example of how to do that. I imagine an enum like:</div>
<div><br></div><div>enum InheritanceBreakingStyle {</div><div> IBS_AfterColon,</div><div> IBS_BeforeColon,</div><div> IBS_BeforeComma</div><div>};</div><div><br></div><div>Does that make sense?</div><div><div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr">
<div>* The parameters themselves have tests (ParsesConfiguration).</div></div></blockquote><div><br></div></div><div>Ok, done.</div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div><br></div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ case tok::semi:<br>+ Contexts.back().CanBeInInheritanceList = false;<br>+ break;</blockquote></div><div><br></div><div>Is this necessary? Can you create a test that breaks without it?</div></div></blockquote>
<div><br></div></div><div>You're right. It wasn't necessary</div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div><br></div><div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> case tok::l_brace:<br>+ Contexts.back().CanBeInInheritanceList = false;<br>
if (!parseBrace())</blockquote></div><div><br></div><div><div>Is this necessary? Can you create a test that breaks without it?</div></div></div></blockquote><div><br></div></div><div>You're right again. It wasn't necessary.</div>
<div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><div><br></div><div></div>
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ //} else if (Contexts.size() == 1) {<br>+ // FIXME: annotates switch-case colon as inheritance colon<br>+ // FIXME: annotates access specifiers as inheritance colon<br>+ // Tok->Type = TT_OtherColon;</blockquote>
<div><br></div><div>Is this intentional?</div></div></blockquote><div><br></div></div><div>I removed this part.</div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div><br></div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
else if (Previous.Type == TT_InheritanceColon) </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
- State.Stack.back().Indent = State.Column;<br>+ // We had a colon on a newline. Now, if we want to align commas to the<br>+ // colon, we indent less by -2 == length(", ")</blockquote></div>
<div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">+ if (Style.BreakClassInheritanceListsBeforeComma)<br>
+ State.Stack.back().Indent = State.Column - 2;<br>+ else<br>+ State.Stack.back().Indent = State.Column;</blockquote><div><br></div><div>Remove this completely. </div></div></div></blockquote><div>
<br></div></div><div>Well, I couldn't remove it completely.</div><div>If you have any idea how to do it in a cleaner way, I'll be glad to know.</div></div></div></div></blockquote><div><br></div></div></div><div>
Yes, you could have. By now, there is one more statement (changing LastSpace), but everything that was in there could have been removed. As this is getting too complicated to explain, I have attached a patch removing everything that I think can be removed (and the tests still pass). Also, I have cleaned up a few basic formatting errors. Interestingly, you don't seem to be using clang-format ;-).</div>
<div><div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ if (Current.Type == TT_InheritanceColon) {<br> State.Stack.back().AvoidBinPacking = true;<br>+ if (!Style.BreakClassInheritanceListsBeforeComma)<br>+ State.Stack.back().Indent = State.Column + 2;<br>
+ } </blockquote><div><br></div><div>Change this to</div></div><div><br></div><div>if (Current.Type == TT_InheritanceColon) {</div><div> State.Stack.back().AvoidBinPacking = true;<br></div><div> State.Stack.back().Indent = State.Column;</div>
<div> if (!Style.BreakClassInheritanceListsBeforeComma)</div><div> State.Stack.back().Indent += 2;</div><div>}</div></div></blockquote><div><br></div></div><div>Done.<br></div><div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="gmail_extra"><div class="gmail_quote"><div><div>
On Fri, Aug 2, 2013 at 10:59 AM, Curdeius Curdeius <span dir="ltr"><<a href="mailto:curdeius@gmail.com" target="_blank" class="cremed">curdeius@gmail.com</a>></span> wrote:<br></div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div>
<div dir="ltr"><div>Hi,</div><div><br></div><div>I've added two options to clang-format:</div><div>* BreakClassInheritanceListsBeforeColon</div><div>and</div><div>* BreakClassInheritanceListsBeforeComma.<br>
</div><div><br></div><div>They allow us to break on class inheritance lists similarly to constructor initializer lists.</div><div><br></div><div>I've corrected as well the TokenAnnotator, which mistakenly annotated colons from switch statements ("case:") and from access modifiers ("public:") as TT_InheritanceColon.</div>
<div><br></div><div>The patch in the attachment.</div><br clear="all"><div>Cheers,<br>Marek Kurdej</div>
</div>
<br></div></div>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank" class="cremed">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank" class="cremed">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br></div>
</blockquote></div></div></div><br><div class="gmail_extra"><br></div><div class="gmail_extra"><div>Regards,<br>Marek Kurdej</div></div></div></blockquote><div><br></div></div></div><div>+ } else if (Current.Type == TT_InheritanceComma) {</div>
<div>+ if (Style.BreakClassInheritanceListsBeforeComma)</div><div>+ State.Stack.back().Indent -= 2; // 2 is length of ", "</div><div>+ }</div><div><br></div><div>I also removed this as it seemed suspicious .. Tests still pass :-).</div>
<div><br></div><div>Please start from the attached patch when continuing to work on this.</div><div><br></div><div>Cheers,</div><div>Daniel</div></div><br></div></div>
</blockquote></div><br></div></div></div></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div></div>