[PATCH] D79388: [clang-format] Fix AlignConsecutive on PP blocks

JakeMerdichAMD added a comment. Belated 'sounds good to me'. Other things have been keeping me busy, sorry for the delayed response on this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79388/new/ https://reviews.llvm.org/D79388

[PATCH] D79388: [clang-format] Fix AlignConsecutive on PP blocks

JakeMerdichAMD added a comment. I'd be very surprised if any of the tests included in this change pass with that line commented it's meant so that things like #if and #else properly separate alignment after the first preprocessor run, where the whitespace manager doesn't have the full

[PATCH] D87587: [clang-format] Make one-line namespaces resistant to FixNamespaceComments, update documentation

JakeMerdichAMD edited reviewers, added: MyDeveloperDay; removed: jmerdich. JakeMerdichAMD requested changes to this revision. JakeMerdichAMD added a subscriber: MyDeveloperDay. JakeMerdichAMD added a comment. This revision now requires changes to proceed. After looking at more of the history (ie,

[PATCH] D87201: [clang-format] Add a option for the position of Java static import

JakeMerdichAMD accepted this revision. JakeMerdichAMD added a comment. This revision is now accepted and ready to land. Sorry on the delay, LGTM too. It looks like you're a first time contributor and probably don't have write access to the repo, do you want one of us to push this on your

[PATCH] D87201: [clang-format] Add a option for the position of Java static import

JakeMerdichAMD added inline comments. Comment at: clang/include/clang/Format/Format.h:1708 + /// \endcode + bool JavaStaticImportAfterImport; + MyDeveloperDay wrote: > Can we consider changing the name or the option to make it clearer what its > for? > >

[PATCH] D87201: [clang-format] Add a option for the position of Java static import

JakeMerdichAMD added inline comments. Comment at: clang/include/clang/Format/Format.h:1705 + /// \endcode + bool JavaStaticImportAfterImport; + bc-lee wrote: > JakeMerdichAMD wrote: > > 3 things here: > > > > 1. Did you mix up the true and false cases? > > 2.

[PATCH] D87201: [clang-format] Add a option for the position of Java static import

JakeMerdichAMD requested changes to this revision. JakeMerdichAMD added inline comments. This revision now requires changes to proceed. Comment at: clang/include/clang/Format/Format.h:1705 + /// \endcode + bool JavaStaticImportAfterImport; + 3 things here: 1.

[PATCH] D86137: Add ignore-unknown-options flag to clang-format.

JakeMerdichAMD added a comment. I can see the use of this, but I am also wary that ignoring style options will lead to people producing different results on different versions of clang-format. This is both because having set-or-unset an option will naturally lead to different code and also

[PATCH] D86713: [clang-format] Parse nullability attributes as a pointer qualifier

JakeMerdichAMD added a comment. In D86713#2242300 , @arichardson wrote: > In D86713#2242253 , @JakeMerdichAMD > wrote: > >> LGTM, again assuming tests pass locally (patch did not resolve). >> >> Out of curiosity,

[PATCH] D86713: [clang-format] Parse nullability attributes as a pointer qualifier

JakeMerdichAMD accepted this revision. JakeMerdichAMD added a comment. This revision is now accepted and ready to land. LGTM, again assuming tests pass locally (patch did not resolve). Out of curiosity, is _Atomic on your radar? I found some code in clang proper that handled restrict and

[PATCH] D86710: [clang-format] Parse restrict as a pointer qualifier

JakeMerdichAMD accepted this revision. JakeMerdichAMD added a comment. This revision is now accepted and ready to land. LGTM, assuming tests pass (automated checks failed to resolve your patch since you based it off of your other one). Looks like enabling C99 should have no other effects,

[PATCH] D86708: [clang-format] Parse volatile as a pointer qualifier

JakeMerdichAMD accepted this revision. JakeMerdichAMD added a comment. This revision is now accepted and ready to land. LGTM. Wait a bit to give others a chance to chime in before submitting. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86708/new/

[PATCH] D86581: [clang-format] Handle shifts within conditions

JakeMerdichAMD added a comment. Agreed that this is a very nice solution for this case. LGTM too assuming it passes @MyDeveloperDay's tests. Minor heads up: there are still some cases I can dream up where we'd currently spit out invalid code from splitting a right shift even after this fix.

[PATCH] D84375: [git-clang-format] Add --diffstat parameter

JakeMerdichAMD added reviewers: MyDeveloperDay, JakeMerdichAMD. JakeMerdichAMD added a comment. Reviving this since it looks perfectly fine to me (from my limited commit history in git-clang-format :P), is useful, and there's no good reason for it to be stalled. I'll wait a day or two to see

[PATCH] D82620: [clang-format] Preserve whitespace in selected macros

This revision was automatically updated to reflect the committed changes. Closed by commit rG0c332a7784c6: [clang-format] Preserve whitespace in selected macros (authored by JakeMerdichAMD). Changed prior to commit: https://reviews.llvm.org/D82620?vs=273722=274102#toc Repository: rG LLVM

[PATCH] D82620: [clang-format] Preserve whitespace in selected macros

JakeMerdichAMD added a comment. Thanks for the fast review, @curdeius, and thanks for mentioning PP_STRINGIZE and BOOST_PP_STRINGIZE too! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82620/new/ https://reviews.llvm.org/D82620

[PATCH] D82620: [clang-format] Preserve whitespace in selected macros

JakeMerdichAMD updated this revision to Diff 273722. JakeMerdichAMD marked 3 inline comments as done. JakeMerdichAMD added a comment. Address feedback (nits, better docs, more defaults) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82620/new/

[PATCH] D82620: [clang-format] Preserve whitespace in selected macros

JakeMerdichAMD marked 10 inline comments as done. JakeMerdichAMD added inline comments. Comment at: clang/docs/ClangFormatStyleOptions.rst:2706 + + For example: STRINGIZE + curdeius wrote: > Shouldn't there be a configuration example like what's in

[PATCH] D82620: [clang-format] Preserve whitespace in selected macros

JakeMerdichAMD created this revision. Herald added subscribers: cfe-commits, kristof.beyls. Herald added a project: clang. JakeMerdichAMD added reviewers: MyDeveloperDay, curdeius, sammccall, jbcoe. JakeMerdichAMD added a project: clang-format. https://bugs.llvm.org/show_bug.cgi?id=46383 When

[PATCH] D80627: [clang-format] Create a python documentation tool to generate a summary of the clang-format status for the whole of the LLVM project

JakeMerdichAMD added a comment. Great idea on this! I may borrow this idea and make something similar for some migrations I'm working on. Sanity check: is this going to be run automatically when docs are generated or done offline with results committed? I don't have a preference, either has

[PATCH] D80486: [clang-format][PR46043] Parse git config w/ implicit values

JakeMerdichAMD created this revision. JakeMerdichAMD added reviewers: MyDeveloperDay, krasimir, sammccall. Herald added a project: clang. Herald added a subscriber: cfe-commits. https://bugs.llvm.org/show_bug.cgi?id=46043 Git's config is generally of the format 'key=val', but a setting

[PATCH] D80079: [clang-format] [NFC] isCpp() is inconsistently used to mean both C++ and Objective C, add language specific isXXX() functions

JakeMerdichAMD added a comment. I'm a fan of the 'like' helpers, but I'm not entirely convinced that having helpers for languages covered by the 'like' is a bad thing-- it just needs to be very explicit that you do mean only that language. For example, an 'isObjCOnly()' would hint to reviewers

[PATCH] D80214: [clang-format] Set of unit test to begin to validate that we don't change defaults

JakeMerdichAMD accepted this revision. JakeMerdichAMD added a comment. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80214/new/ https://reviews.llvm.org/D80214 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D80214: [clang-format] Set of unit test to begin to validate that we don't change defaults

JakeMerdichAMD added a comment. Just belatedly caught something: Webkit style is supported too but not listed here. Can you add that? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80214/new/ https://reviews.llvm.org/D80214

[PATCH] D80309: [clang-format][docfix] Update predefined styles in docs

JakeMerdichAMD created this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. JakeMerdichAMD added reviewers: MyDeveloperDay, krasimir, mitchell-stellar, sammccall. JakeMerdichAMD added a project: clang-format. The predefined styles that clang-format supports are

[PATCH] D80214: [clang-format] Set of unit test to begin to validate that we don't change defaults

JakeMerdichAMD added a comment. +1 for this idea. It'd eventually be neat to also take all samples from the style guide of each project and test them, if there aren't licensing concerns. LGTM with an appropriate merge fix/CI passing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D79465: [clang-format] Fix line lengths w/ comments in align

JakeMerdichAMD added a comment. Hey @MyDeveloperDay, can I get your assistance committing this when you have the chance? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79465/new/ https://reviews.llvm.org/D79465

[PATCH] D80176: [clang-format][PR45816] Add AlignConsecutiveBitFields

JakeMerdichAMD added a comment. Can I get your assistance committing this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80176/new/ https://reviews.llvm.org/D80176 ___ cfe-commits mailing list

[PATCH] D80176: [clang-format][PR45816] Add AlignConsecutiveBitFields

JakeMerdichAMD updated this revision to Diff 265048. JakeMerdichAMD added a comment. Reformat with a newer clang-format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80176/new/ https://reviews.llvm.org/D80176 Files:

[PATCH] D80144: [clang-format] @lefticus just taught the world how to use [[unlikely]] but we forgot to teach clang-format

JakeMerdichAMD accepted this revision. JakeMerdichAMD added a comment. This revision is now accepted and ready to land. LGTM assuming CI tests pass. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80144/new/ https://reviews.llvm.org/D80144

[PATCH] D80115: [clang-format] [PR45816] Add AlignConsecutiveBitFields

JakeMerdichAMD added a comment. See follow-up in D80176 . CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80115/new/ https://reviews.llvm.org/D80115 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D80176: [clang-format][PR45816] Add AlignConsecutiveBitFields

JakeMerdichAMD created this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. The following revision follows https://reviews.llvm.org/D80115 since @MyDeveloperDay and I apparently both had the same idea at the same time, for

[PATCH] D80144: [clang-format] @lefticus just taught the world how to use [[unlikely]] but we forgot to teach clang-format

JakeMerdichAMD added a comment. This is a great improvement in readability. I think this will get in here before it does for clang proper :D Likely/unlikely also seem to be supported on while, do-while, and for loops (case statements too, but that's not relevant). Should we also apply

[PATCH] D80041: [clang-format] [PR45198] deletes whitespace inside of a noexcept specifier

JakeMerdichAMD added a comment. I mainly have concerns with the search for noexcept being too brittle and not looking far enough back. Implementing that may have performance implications though, so I have no issue ignoring that problem if it's proven sufficiently rare. Also: should this do

[PATCH] D80115: [clang-format] [PR45816] Add AlignConsecutiveBitFields

JakeMerdichAMD added a comment. Good idea, feel free to incorporate anything about the change (eg, tests and TT_BitFieldColon) or delegate to me if you like. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80115/new/ https://reviews.llvm.org/D80115

[PATCH] D79905: [clang-format] [PR44476] Add space between template and attribute

JakeMerdichAMD accepted this revision. JakeMerdichAMD added a comment. This revision is now accepted and ready to land. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79905/new/ https://reviews.llvm.org/D79905 ___ cfe-commits mailing

[PATCH] D80115: [clang-format] [PR45816] Add AlignConsecutiveBitFields

JakeMerdichAMD added a comment. Ironically, I independently made a near-identical change because my codebase needs this, and was going to upload it today. I won't post it now, but I stuck a copy on github if you're interested:

[PATCH] D79465: [clang-format] Fix line lengths w/ comments in align

JakeMerdichAMD updated this revision to Diff 264294. JakeMerdichAMD added a comment. Rebase to fix merge conflict Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79465/new/ https://reviews.llvm.org/D79465 Files:

[PATCH] D79465: [clang-format] Fix line lengths w/ comments in align

JakeMerdichAMD updated this revision to Diff 264259. JakeMerdichAMD added a comment. Add a comment explaining why checking IsInsideToken is needed here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79465/new/ https://reviews.llvm.org/D79465

[PATCH] D79905: [clang-format] [PR44476] Add space between template and attribute

JakeMerdichAMD added a comment. One spelling nit, but otherwise looks good to me. Comment at: clang/lib/Format/TokenAnnotator.cpp:2900 + + // Space between tempalte and attribute + // e.g. template [[nodiscard]] ... Nit: template Repository: rG LLVM

[PATCH] D79388: [clang-format] Fix AlignConsecutive on PP blocks

JakeMerdichAMD added a comment. Thanks for the commit and review @MyDeveloperDay! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79388/new/ https://reviews.llvm.org/D79388 ___ cfe-commits mailing list

[PATCH] D79388: [clang-format] Fix AlignConsecutive on PP blocks

JakeMerdichAMD added a comment. Hey, @MyDeveloperDay, can I get your assistance in committing this? It's probably been long enough for anyone to chime in. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79388/new/ https://reviews.llvm.org/D79388

[PATCH] D79465: Fix line lengths w/ comments in align

JakeMerdichAMD added a comment. Mind looking again? They did add one later on I think. It helped a lot since the exact settings and whitespace to trigger this are really finicky (though reproducible). You're right that it won't reproduce without it. Here's a run log of the failing test I

[PATCH] D79465: Fix line lengths w/ comments in align

JakeMerdichAMD added a project: clang-format. JakeMerdichAMD added reviewers: MyDeveloperDay, krasimir, VelocityRa, sylvestre.ledru, sammccall, enyquist. JakeMerdichAMD added a comment. The failing case in this commit looks like the following after formatting (with alignconsecutiveassignments

[PATCH] D79465: Fix line lengths w/ comments in align

JakeMerdichAMD created this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. JakeMerdichAMD added a project: clang-format. JakeMerdichAMD added reviewers: MyDeveloperDay, krasimir, VelocityRa, sylvestre.ledru, sammccall, enyquist. JakeMerdichAMD added a comment.

[PATCH] D79388: [clang-format] Fix AlignConsecutive on PP blocks

JakeMerdichAMD added a comment. Sure, I'll get started on that. It mainly comes from charging headfirst into the edge cases, but I think I have a decent grasp of clang-format internals now, and I'm definitely interested in both contributing to and reviewing for it. Repository: rG LLVM

[PATCH] D79388: [clang-format] Fix AlignConsecutive on PP blocks

JakeMerdichAMD added a comment. @MyDeveloperDay, you're right about what this issue addresses: it surprised me a lot when an unrelated edit caused something to 'randomly' add spaces elsewhere, since it's in a different ifdef block (whether or not it has the same condition). The code that's

[PATCH] D79388: [clang-format] Fix AlignConsecutive on PP blocks

JakeMerdichAMD created this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Currently the 'AlignConsecutive*' options incorrectly align across elif and else statements, even if they are very far away and across unrelated preprocessor macros. This failed since on