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