[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

nikola added a comment. Commit r298574, thanks for woking on this folks! https://reviews.llvm.org/D21279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

daphnediane added a comment. ping https://reviews.llvm.org/D21279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

bmharper added a comment. Hi @djasper, This is the first patch I've contributed here, so I'm not familiar with the whole process. I assume this code is ready to land? When exactly does it get merged into master, and is there something else that I still need to do to make that happen? Thanks, B

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

bmharper updated this revision to Diff 89463. bmharper added a comment. Fixed two small issues raised by @daphnediane https://reviews.llvm.org/D21279 Files: lib/Format/WhitespaceManager.cpp lib/Format/WhitespaceManager.h unittests/Format/FormatTest.cpp Index: unittests/Format/FormatTest.

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

daphnediane added a comment. Rebuilt with the latest patch and got one compile error. See line comment worked okay after fixing it. Comment at: lib/Format/WhitespaceManager.cpp:215 + +if (i != Start) { + if (Changes[i].nestingAndIndentLevel() > These

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

bmharper added a comment. Thanks for all the code review work! I'm impressed by the quality standard maintained here. https://reviews.llvm.org/D21279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/list

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

bmharper updated this revision to Diff 87478. bmharper added a comment. small comment tweak https://reviews.llvm.org/D21279 Files: lib/Format/WhitespaceManager.cpp lib/Format/WhitespaceManager.h unittests/Format/FormatTest.cpp Index: unittests/Format/FormatTest.cpp ==

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

djasper accepted this revision. djasper added a comment. This looks very nice now :-D. Thanks for working on this!! Comment at: lib/Format/WhitespaceManager.cpp:196 + + // ScopeStack keeps track of the current scope depth. + // We only run the "Matches" function on tokens fro

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

bmharper updated this revision to Diff 87361. bmharper added a comment. This looks a lot better IMO. Thanks @daphnediane - you should recognize the code ;) The special closing paren logic inside of nestingAndIndentLevel() is indeed redundant now. https://reviews.llvm.org/D21279 Files: lib/F

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

daphnediane added a comment. In https://reviews.llvm.org/D21279#667471, @bmharper wrote: > Thanks @daphnediane. I only read your comment after merging, but that would > have been helpful. If you still want my diff let me know as it is slightly different from yours. No longer has NestingAndInd

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

djasper added inline comments. Comment at: lib/Format/WhitespaceManager.cpp:190 +struct TokenTypeAndLevel { + TokenType Type; I don't think you need this struct now. Just use the FormatToken itself, it should have all of this information. C

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

bmharper added a comment. Thanks @daphnediane. I only read your comment after merging, but that would have been helpful. https://reviews.llvm.org/D21279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/l

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

bmharper updated this revision to Diff 87179. bmharper added a comment. Fixed a stale comment https://reviews.llvm.org/D21279 Files: lib/Format/WhitespaceManager.cpp lib/Format/WhitespaceManager.h unittests/Format/FormatTest.cpp Index: unittests/Format/FormatTest.cpp

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

bmharper updated this revision to Diff 87178. bmharper added a comment. Hi @djasper, I've made the latest bunch of review changes, and rebased onto master. I didn't check the numbers, but I think the code is slightly smaller after the rebase. Regards, Ben https://reviews.llvm.org/D21279 Files

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

daphnediane added a comment. In https://reviews.llvm.org/D21279#663670, @bmharper wrote: > Thanks - the merge conflicts don't look too bad. I should have it cleaned up > by tomorrow. Have you had a chance to do the merge yet? If not I have a merged version which passes the tests that I could

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

bmharper added a comment. Thanks - the merge conflicts don't look too bad. I should have it cleaned up by tomorrow. https://reviews.llvm.org/D21279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listin

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

djasper added a comment. I apologize in advance if this causes merge conflicts with r293616. However, I do hope that that actually makes this patch easier. https://reviews.llvm.org/D21279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

djasper added inline comments. Comment at: lib/Format/WhitespaceManager.cpp:207 + +if (i != Start) { + if (Changes[i].NestingAndIndentLevel > Merge the two ifs into a single one? Comment at: lib/Format/WhitespaceManager.cpp:318 + for

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

bmharper added a comment. Pinging @djasper. Any chance we can get this merged? https://reviews.llvm.org/D21279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

enyquist added a comment. Well, your patch is here for me to try, and it looks like it's been accepted. So I guess I should just pull my finger out and try it :) Thanks for your response-- I'll let you know if I come across any issues. https://reviews.llvm.org/D21279

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

bmharper added a comment. Hi @enyquist, I'd like to guess that this patch will solve your problem, but I'm not intimate enough with this code to give you a definitive answer. I hope we can get this merged soon, so that you can just run it and see. Ben https://reviews.llvm.org/D21279 __

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

enyquist added a comment. Hey bmharper :) I've got a review open that conflicts with this one, just having a look to see what I'll need to refactor (https://reviews.llvm.org/D28462). In fact, I have a question-- the conflict is specifically in WhitespaceManager.cpp. Since I needed to detect PP

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

berenm added a comment. Pinging @djasper There's https://reviews.llvm.org/D27651 that will conflict with this one. https://reviews.llvm.org/D21279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinf

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

bmharper updated this revision to Diff 81213. bmharper added a comment. Sorry for the incredibly long turnaround time on this one - I don't have any legitimate excuse, and I know it just makes reviewing it harder. Anyway - I have sorted out all the issues mentioned in the last review. One thing

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

djasper added a comment. So sorry. Seems I forgot to hit "Submit" :(. If you don't like the ".first" and ".second" of the pair, you could introduce a struct for it and overload operator<. Might actually be more readable. > WhitespaceManager.cpp:73 > + Tok.NestingLevel, >/*Spaces=

[PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

bmharper added a comment. ping! https://reviews.llvm.org/D21279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

bmharper updated this revision to Diff 72444. bmharper added a comment. This revision merges NestingLevel and IndentLevel into an std::pair, as suggested by @djasper. IMO this makes the code slightly harder to read, because you lose the unique variable names "IndentLevel" and "NestingLevel". Tho

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

berenm added a comment. In https://reviews.llvm.org/D21279#550565, @djasper wrote: > Are we talking completely past each other? I specifically think we should > *NOT* combine NestingLevel and IndentLevel into one value. Not in > ScopeLevel() and not anywhere else. Ok, I probably misunderstood

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

djasper added a comment. Are we talking completely past each other? I specifically think we should *NOT* combine NestingLevel and IndentLevel into one value. Not in ScopeLevel() and not anywhere else. https://reviews.llvm.org/D21279 ___ cfe-commit

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

berenm added a comment. Hello, I had a little bit of look into the NestingLevel field. I understand that it only indicates the nesting level of the token inside the current unwrapped line, which could very well be the same as the nesting level of another token in the previous or next unwrapped

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

bmharper added a comment. So.. I finally got some time to look at this again: Quick Recap - IndentLevel and NestingLevel are now stored separately inside WhitespaceManager::Change. I've added a function ScopeLevel() which combines them with a bit of logic, and returns a number that can be used

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

nikola added a subscriber: nikola. Comment at: lib/Format/WhitespaceManager.cpp:356 @@ -273,3 +355,3 @@ // If there is more than one matching token per line, or if the number of // preceding commas, or the scope depth, do not match anymore, end the // sequence.

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

djasper added inline comments. Comment at: lib/Format/WhitespaceManager.cpp:47 @@ +46,3 @@ + +int WhitespaceManager::Change::ScopeLevel() const { + // NestingLevel is raised on the opening paren/square, and remains raised What I don't understand is why you have t

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

bmharper added a comment. PING! My previous commit hopefully addressed the issue with the sprawl of IndentLevel + ScopeLevel https://reviews.llvm.org/D21279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailm

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

bmharper updated this revision to Diff 69671. bmharper added a comment. I have added an initial phase which propagates IndentLevel from the first token on a line, to all of the tokens on that line. This allows us to get rid of the ScopeLevel state. However, I have retained the name "ScopeLevel",

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

bmharper added a comment. I'll see what happens if I make IndentLevel the same for all tokens on a line. If not too much is broken by that, I should be able to handle it. https://reviews.llvm.org/D21279 ___ cfe-commits mailing list cfe-commits@list

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

djasper added a comment. I think the IndentLevel in WhitespaceManager (and the nested Change) is a horrible mess and should be cleaned up. It gets set either to 0 or to the "Level" of the AnnotatedLine. To me only the latter makes sense as the line defines the indent level. Everything else, inc

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

bmharper added a comment. The reason one has to precompute ScopeLevel is because IndentLevel is not actually meaningful on each token. It's only meaningful for the first token on the line (the remaining tokens on the line have IndentLevel = 0). So if you look at the implementation of calculateS

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

djasper added a comment. I think instead of doing some complex computation with LineLevel and NestingLevel, it might be better to just leave them as the pair and compare them as a pair. The LineLevel should probably always trump the NestingLevel. So, I'd try to just defined ScopeLevel as a pair

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

bmharper removed rL LLVM as the repository for this revision. bmharper updated this revision to Diff 68675. bmharper added a comment. Fixed the two issues pointed out by djasper in his most recent comments: 1. Only calculate ScopeLevel when necessary. 2. Instead of calculating ScopeLevel by inspe

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

djasper added a comment. Sorry :(... Review is easy enough.. Feel free to ping me more often in the future. Comment at: lib/Format/WhitespaceManager.cpp:95 @@ -97,2 +94,3 @@ std::sort(Changes.begin(), Changes.end(), Change::IsBeforeInFile(SourceMgr)); + calculateScopeLevel(

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

PING. Is there anything I can do to make reviewing easier? For example, I could run my code vs master on some large code bases (eg linux kernel, chromium), and verify that there are no crashes, and also manually spot check some results. Ben On Wed, 20 Jul 2016 at 21:43 Ben Harper wrote: > bmh

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

bmharper added a comment. PING Comment at: lib/Format/WhitespaceManager.cpp:95 @@ -97,2 +94,3 @@ std::sort(Changes.begin(), Changes.end(), Change::IsBeforeInFile(SourceMgr)); + calculateScopeLevel(); calculateLineBreakInformation(); berenm wrote: > Maybe

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

bmharper added a comment. kaPING! Repository: rL LLVM http://reviews.llvm.org/D21279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

djasper added a comment. Sorry.. Really busy at the moment, but will try to get this reviewed and submitted this week. If not, please ping again! Repository: rL LLVM http://reviews.llvm.org/D21279 ___ cfe-commits mailing list cfe-commits@lists.l

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

bmharper added a comment. Friendly PING. Please let me know if there's anything else that I need to do here, otherwise I'll keep quiet and expect a merge at some point? Repository: rL LLVM http://reviews.llvm.org/D21279 ___ cfe-commits mailing l

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

bmharper added a subscriber: bmharper. bmharper added a comment. Hi Daniel, Is there anything else that I need to do here? Regards, Ben Repository: rL LLVM http://reviews.llvm.org/D21279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org ht

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

berenm added a comment. In http://reviews.llvm.org/D21279#462578, @bmharper wrote: > 2. friend functions: I don't really understand why the current behavior is > what it is, but I think it's reasonable to argue that it actually improves > readability by drawing attention to the fact these are f

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

bmharper added a comment. I've taken some time to investigate those two issues, and these are my thoughts: 1. Constructor alignment: I think this is a good thing to do, but if `isFunctionDeclarationName`, and it's caller `TokenAnnotator::calculateFormattingInformation` are anything to go by, ad

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

berenm added a comment. Thanks! The operators are now correctly handled. Another thing I've found is that constructors aren't aligned either with other declarations or either together. Do you think it would be possible to treat them as functions as well? Friend functions also aren't aligned wi

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

bmharper set the repository for this revision to rL LLVM. bmharper updated this revision to Diff 60830. bmharper added a comment. Fix the recent two issues mentioned by Beren, ie the single-statement scopes (for loop without braces), and operator[] alignment. Repository: rL LLVM http://revie

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

bmharper added a comment. Interesting. Working on it! http://reviews.llvm.org/D21279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

berenm added a comment. It looks like it doesn't like the `operator[]` either: struct test { long long int foo(); int operator[](int a); double bar(); long long int foo(); int operator()(int a); double bar(); }; becomes: struct test { long long int foo();

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

berenm added a comment. There's still cases where the nesting level is still not correctly handled: when using unbraced conditionals / loops. For example (sorry, silly example): for (auto index = 0, e = 1000; index < e; ++index) int v = 0; long double value = 1; is aligned to for (au

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

bmharper removed rL LLVM as the repository for this revision. bmharper updated this revision to Diff 60569. bmharper added a comment. diff with full context http://reviews.llvm.org/D21279 Files: lib/Format/WhitespaceManager.cpp lib/Format/WhitespaceManager.h unittests/Format/FormatTest.cp

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

berenm added a comment. I think it's better to update the diff with full context (git diff -U99) rather than attach the file, in order to have it integrated into phabricator diff view. Or use the arcanist command-line tool, which should do it for you. Except from that, from a quick reading

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

bmharper added a comment. As requested - full diff (of all three files) F2059068: AlignFullDiff.patch Repository: rL LLVM http://reviews.llvm.org/D21279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org h

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

djasper added a comment. Could you upload a diff with full (i.e. the entire file as) context? Repository: rL LLVM http://reviews.llvm.org/D21279 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinf

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

kfunk added a subscriber: kfunk. kfunk added a comment. In http://reviews.llvm.org/D21279#455923, @klimek wrote: > Generally, please subscribe cfe-commits when sending patches via phab. > See http://llvm.org/docs/Phabricator.html You should set up a Herald rule then: https://secure.phabricato

Re: [PATCH] D21279: Fix some issues in clang-format's AlignConsecutive modes

klimek added a subscriber: cfe-commits. klimek added a comment. Generally, please subscribe cfe-commits when sending patches via phab. See http://llvm.org/docs/Phabricator.html Repository: rL LLVM http://reviews.llvm.org/D21279 ___ cfe-commits ma