Somehow cfe-commits got lost.. Adding that back to CC..
On Thu, Oct 24, 2013 at 9:41 AM, Daniel Jasper <[email protected]> wrote: > Yes you are right, I was confused. However, my confusion was stemming from > the fact that your tests don't include the interesting case: > > +TEST_F(FormatTest, BreaksBeforeClassInheritanceComma) { > + FormatStyle Style = getWebKitStyle(); > + Style.BreakClassInheritanceListsBeforeComma = true; > + > + verifyFormat("namespace outer {\n" > + "int i;\n" > + "namespace inner {\n" > + " int i;\n" > + " class Aaaa : public Bbb\n" > + " , protected Ccc {\n" > > 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). > > 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: > 1) Always break before the colon > 2) Break before the colon if the entire line (up to "{") doesn't fit into > the column limit > 3) Break before the colon if the first base class (i.e. everything up to > ",") does not fit on one line > 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 > > 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. > > 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. > > Cheers and sorry for not spotting this earlier! > Daniel > > > On Thu, Oct 24, 2013 at 9:24 AM, Curdeius Curdeius <[email protected]>wrote: > >> Hi Daniel, >> thanks again. >> Just a small remark. >> You say I'm not testing all the 4 possible cases, but (in my opinion), I >> do. >> I created 3 tests: >> >> - TEST_F(FormatTest, BreaksBeforeClassInheritanceColonAndComma) - >> true, true case >> - TEST_F(FormatTest, BreaksBeforeClassInheritanceComma) - false, true >> case >> - TEST_F(FormatTest, BreaksBeforeClassInheritanceColon) = true, false >> case >> >> The 4th case (false, false) is taken into account by existing tests. >> >> I'll take into account and write back ASAP. >> >> Cheers, >> Marek >> >> >> 2013/10/23 Daniel Jasper <[email protected]> >> >>> >>> >>> >>> On Wed, Oct 23, 2013 at 3:44 PM, Curdeius Curdeius >>> <[email protected]>wrote: >>> >>>> Hi Daniel, >>>> thanks for your feedback and sorry for the late response. >>>> >>>> I attach the patch. >>>> >>>> My comments to your remarks below. >>>> >>>> * How is the new test in FormatsWithWebKitStyle related? >>>>> >>>> >>>> I removed the unnecessary mess. >>>> >>>> >>>>> * Either collapse both parameters into a single parameter or add test >>>>> cases for all combinations. >>>>> >>>> >>>> I did so. >>>> >>> >>> 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: >>> >>> enum InheritanceBreakingStyle { >>> IBS_AfterColon, >>> IBS_BeforeColon, >>> IBS_BeforeComma >>> }; >>> >>> Does that make sense? >>> >>> >>>> >>>>> * The parameters themselves have tests (ParsesConfiguration). >>>>> >>>> >>>> Ok, done. >>>> >>>> >>>>> >>>>> + case tok::semi: >>>>>> + Contexts.back().CanBeInInheritanceList = false; >>>>>> + break; >>>>> >>>>> >>>>> Is this necessary? Can you create a test that breaks without it? >>>>> >>>> >>>> You're right. It wasn't necessary >>>> >>>> >>>>> >>>>> case tok::l_brace: >>>>>> + Contexts.back().CanBeInInheritanceList = false; >>>>>> if (!parseBrace()) >>>>> >>>>> >>>>> Is this necessary? Can you create a test that breaks without it? >>>>> >>>> >>>> You're right again. It wasn't necessary. >>>> >>>> >>>>> >>>>> + //} else if (Contexts.size() == 1) { >>>>>> + // FIXME: annotates switch-case colon as inheritance colon >>>>>> + // FIXME: annotates access specifiers as inheritance colon >>>>>> + // Tok->Type = TT_OtherColon; >>>>> >>>>> >>>>> Is this intentional? >>>>> >>>> >>>> I removed this part. >>>> >>>> >>>>> >>>>> else if (Previous.Type == TT_InheritanceColon) >>>>> >>>>> - State.Stack.back().Indent = State.Column; >>>>>> + // We had a colon on a newline. Now, if we want to align >>>>>> commas to the >>>>>> + // colon, we indent less by -2 == length(", ") >>>>> >>>>> + if (Style.BreakClassInheritanceListsBeforeComma) >>>>>> + State.Stack.back().Indent = State.Column - 2; >>>>>> + else >>>>>> + State.Stack.back().Indent = State.Column; >>>>> >>>>> >>>>> Remove this completely. >>>>> >>>> >>>> Well, I couldn't remove it completely. >>>> If you have any idea how to do it in a cleaner way, I'll be glad to >>>> know. >>>> >>> >>> 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 ;-). >>> >>> >>>>> + if (Current.Type == TT_InheritanceColon) { >>>>>> State.Stack.back().AvoidBinPacking = true; >>>>>> + if (!Style.BreakClassInheritanceListsBeforeComma) >>>>>> + State.Stack.back().Indent = State.Column + 2; >>>>>> + } >>>>> >>>>> >>>>> Change this to >>>>> >>>>> if (Current.Type == TT_InheritanceColon) { >>>>> State.Stack.back().AvoidBinPacking = true; >>>>> State.Stack.back().Indent = State.Column; >>>>> if (!Style.BreakClassInheritanceListsBeforeComma) >>>>> State.Stack.back().Indent += 2; >>>>> } >>>>> >>>> >>>> Done. >>>> >>>> >>>>> On Fri, Aug 2, 2013 at 10:59 AM, Curdeius Curdeius < >>>>> [email protected]> wrote: >>>>> >>>>>> Hi, >>>>>> >>>>>> I've added two options to clang-format: >>>>>> * BreakClassInheritanceListsBeforeColon >>>>>> and >>>>>> * BreakClassInheritanceListsBeforeComma. >>>>>> >>>>>> They allow us to break on class inheritance lists similarly to >>>>>> constructor initializer lists. >>>>>> >>>>>> I've corrected as well the TokenAnnotator, which mistakenly annotated >>>>>> colons from switch statements ("case:") and from access modifiers >>>>>> ("public:") as TT_InheritanceColon. >>>>>> >>>>>> The patch in the attachment. >>>>>> >>>>>> Cheers, >>>>>> Marek Kurdej >>>>>> >>>>>> _______________________________________________ >>>>>> cfe-commits mailing list >>>>>> [email protected] >>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>>>> >>>>>> >>>>> >>>> >>>> Regards, >>>> Marek Kurdej >>>> >>> >>> + } else if (Current.Type == TT_InheritanceComma) { >>> + if (Style.BreakClassInheritanceListsBeforeComma) >>> + State.Stack.back().Indent -= 2; // 2 is length of ", " >>> + } >>> >>> I also removed this as it seemed suspicious .. Tests still pass :-). >>> >>> Please start from the attached patch when continuing to work on this. >>> >>> Cheers, >>> Daniel >>> >>> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
