<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">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 class="im"><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 class="im"><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>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><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 class="im"><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 class="im"><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 class="im"><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 class="im">
<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 class="im"><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>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><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 class="im"><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 class="im"><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>+ } 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>