[PATCH] D28462: clang-format: Add new style option AlignConsecutiveMacros

klimek added inline comments. Comment at: lib/Format/WhitespaceManager.cpp:520 + + AlignMacroSequence(StartOfSequence, EndOfSequence, MinColumn, MaxColumn, + FoundMatchOnLine, AlignMacrosMatches, Changes); Why are we calling AlignMacroSequenc

[PATCH] D59309: [clang-format] BreakAfterReturnType ignored on functions with numeric template parameters

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. lg Comment at: clang/lib/Format/TokenAnnotator.cpp:3177-3178 return false; // Don't split tagged template literal so there is a break between the tag // iden

[PATCH] D60374: clang-format incorrectly indents wrapped closing parenthesis

klimek added a comment. The previous behavior looks intentional, and much more regular. I'd be curious why you think the proposed behavior is more readable. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60374/new/ https://reviews.llvm.org/D60374 _

[PATCH] D60320: [clang-format]: Add option to insert space after locical not operator

klimek accepted this revision. klimek added a comment. lg CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60320/new/ https://reviews.llvm.org/D60320 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailma

[PATCH] D52527: [clang-format] fix Bug 38686: add AfterCaseLabel to BraceWrapping

klimek accepted this revision. klimek added a comment. Apart from the typo I think this is a simple enough change and a widely enough used style that it LG. Comment at: lib/Format/UnwrappedLineParser.cpp:181 + CompoundStatementIndenter(UnwrappedLineParser *Parser, unsigned &L

[PATCH] D59087: [clang-format] [PR25010] AllowShortIfStatementsOnASingleLine not working if an "else" statement is present

klimek added inline comments. Comment at: cfe/trunk/docs/ClangFormatStyleOptions.rst:384 + + * ``SIS_WithoutElse`` (in configuration: ``WithoutElse``) +Allow short if functions on the same line, as long as else Is that actually the status quo or is the curre

[PATCH] D59746: [LibTooling] Fix '-h' option

klimek added a comment. In D59746#1440756 , @hintonda wrote: > A better alternative would have been to add a cl::aliasopt for '-h' in llvm's > CommandLineParser when '-help' was first added. However, that's no longer > possible since some llvm based too

[PATCH] D59087: [clang-format] [PR25010] AllowShortIfStatementsOnASingleLine not working if an "else" statement is present

klimek added inline comments. Comment at: cfe/trunk/docs/ClangFormatStyleOptions.rst:384 + + * ``SIS_WithoutElse`` (in configuration: ``WithoutElse``) +Allow short if functions on the same line, as long as else MyDeveloperDay wrote: > klimek wrote: > > Is th

[PATCH] D59746: [LibTooling] Fix '-h' option

klimek added a comment. In D59746#1458424 , @hintonda wrote: > In D59746#1458086 , @klimek wrote: > > > In D59746#1440756 , @hintonda > > wrote: > > > > > A better alternati

[PATCH] D60362: [clang-format] [PR39719] clang-format converting object-like macro to function-like macro

klimek added inline comments. Comment at: clang/lib/Format/TokenAnnotator.cpp:2467-2470 + if (Line.InPPDirective && Right.is(tok::l_paren) && + !Left.is(tok::identifier) && Left.Previous && + Left.Previous->is(tok::identifier) && Left.Previous->Previous && + Left.

[PATCH] D59332: [clang-format] AlignConsecutiveDeclarations fails with attributes

klimek added inline comments. Comment at: clang/lib/Format/WhitespaceManager.cpp:468 C.Tok->is(TT_FunctionDeclarationName) || + C.Tok->startsSequence( + tok::l_paren, tok::l_paren, tok::identifier, tok::coloncolon,

[PATCH] D59408: [clang-format] [PR25010] Extend AllowShortIfStatementsOnASingleLine not working if an "else" statement is present

klimek added inline comments. Comment at: clang/docs/ClangFormatStyleOptions.rst:401 + * ``SIS_AlwaysNoElse`` (in configuration: ``AlwaysNoElse``) +Allow short if/else if statements even if the else is a compound statement. + MyDeveloperDay wrote: > klimek w

[PATCH] D60225: [clang-format] [PR19056] Add support for indenting class members and methods one level under the modifiers

klimek added inline comments. Comment at: clang/include/clang/Format/Format.h:50 struct FormatStyle { + /// Indents after access modifiers. i.e. + /// \code I think we need to explain this a bit more: What this does is: Indent classes with access modifiers at

[PATCH] D59746: [LibTooling] Fix '-h' option

klimek added a comment. In D59746#1458539 , @hintonda wrote: > In D59746#1458461 , @hintonda wrote: > > > In D59746#1458432 , @klimek wrote: > > > > > If we make it an alias

[PATCH] D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range

klimek added a comment. In D54881#1449848 , @russellmcc wrote: > klimek: I'm sorry, I don't fully understand your proposed fix. Could you > explain it in more detail? > > Without being able to respond to your proposal in detail, I strongly believe > if

[PATCH] D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range

klimek added a comment. In D54881#1462804 , @russellmcc wrote: > Thanks for the explanation! I do understand your philosophy on this, and > agree with the desired behavior case you brought up where you have put in new > braces. > > After thinking about

[PATCH] D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range

klimek added a comment. In D54881#1462893 , @MyDeveloperDay wrote: > > I agree and would be happy with the change if it would only change the > > line-filtered workflow, but this afaict (unless I'm missing something :) > > will also affect the workflow

[PATCH] D59746: [CommandLineParser] Add DefaultOption flag

klimek added inline comments. Comment at: llvm/lib/Support/CommandLine.cpp:101 + SmallVector DefaultOptions; + A comment explaining what this contains and how it'll be used would help. Comment at: llvm/lib/Support/CommandLine.cpp:152 +

[PATCH] D59746: [CommandLineParser] Add DefaultOption flag

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. lg Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59746/new/ https://reviews.llvm.org/D59746 ___ c

[PATCH] D41911: [clangd] Include scanner that finds compile commands for headers.

klimek added inline comments. Comment at: clangd/IncludeScanner.cpp:75 + bool WasEmpty = Queue.empty(); + for (const auto &Cmd : Cmds) { +QueueEntry E(Cmd, VFS); Usually I'd try to not lock a loop, as a large number of compile commands now blocks other thr

[PATCH] D37813: clang-format: better handle namespace macros

klimek added a comment. In https://reviews.llvm.org/D37813#1253813, @Typz wrote: > In https://reviews.llvm.org/D37813#1184051, @klimek wrote: > > > The problem here is that we have different opinions on how the formatting > > on namespace macros should behave in the first place- I think they sho

[PATCH] D28462: clang-format: Add new style option AlignConsecutiveMacros

klimek added a comment. In D28462#1405855 , @MyDeveloperDay wrote: > In D28462#1405711 , @klimek wrote: > > > In D28462#1405360 , > > @MyDeveloperDay wrote: > > > > > I'm n

[PATCH] D28462: clang-format: Add new style option AlignConsecutiveMacros

klimek added a comment. In D28462#1405360 , @MyDeveloperDay wrote: > I'm not a code owner here but we seem to be lacking people who are either > available, able, willing or allowed to approve code reviews for some of the > code in the clang-tooling (not

[PATCH] D58345: [clangd] Using symbol name to map includes for STL symbols.

klimek added a comment. In D58345#1406892 , @sammccall wrote: > Sorry to be a pain here, but I'm not sure introducing Javascript is a good > idea unless there's a strong reason for it. > All LLVM developers have python installed, many are comfortable wit

[PATCH] D36075: [refactor] Initial support for refactoring action rules

klimek added inline comments. Comment at: cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRule.h:23-37 +/// A common refactoring action rule interface. +class RefactoringActionRule { +public: + enum RuleKind { SourceChangeRefactoringRuleKind }; + + RuleKind getRul

[PATCH] D36075: [refactor] Initial support for refactoring action rules

klimek added inline comments. Comment at: cfe/trunk/include/clang/Tooling/Refactoring/RefactoringActionRule.h:23-37 +/// A common refactoring action rule interface. +class RefactoringActionRule { +public: + enum RuleKind { SourceChangeRefactoringRuleKind }; + + RuleKind getRul

[PATCH] D37291: [refactor] Use a RefactoringResultConsumer instead of tagged refactoring rule classes

klimek added a comment. Thanks! I think this makes the code easier to understand. Now my remaining question is why the ResultType is templates vs. also being an interface (sorry for being late, was out on vacation the past 2 weeks :) Comment at: include/clang/Tooling/Refactor

[PATCH] D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action

klimek added a comment. One of my main concerns is still that I don't see the need for all the template magic yet :) Why doesn't everybody use the RefactoringResult we define here? Comment at: test/Refactor/LocalRename/Field.cpp:4 +class Baz { + int /*range=*/Foo; // CHECK: s

[PATCH] D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action

klimek added a comment. In https://reviews.llvm.org/D36574#860197, @arphaman wrote: > In https://reviews.llvm.org/D36574#858763, @klimek wrote: > > > One of my main concerns is still that I don't see the need for all the > > template magic yet :) Why doesn't everybody use the RefactoringResult w

[PATCH] D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action

klimek added inline comments. Comment at: test/Refactor/LocalRename/Field.cpp:4 +class Baz { + int /*range=*/Foo; // CHECK: symbol [[@LINE]]:17 -> [[@LINE]]:20 +public: arphaman wrote: > klimek wrote: > > arphaman wrote: > > > klimek wrote: > > > > Does this jus

[PATCH] D37662: [AST] Make RecursiveASTVisitor visit TemplateDecls in source order

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. LG https://reviews.llvm.org/D37662 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-com

[PATCH] D37663: [AST] Make RecursiveASTVisitor visit CXXOperatorCallExpr in source order

klimek added inline comments. Comment at: include/clang/AST/RecursiveASTVisitor.h:334 +case OO_Arrow: +case OO_Call: +case OO_Subscript: Why do we need to swap for calls? https://reviews.llvm.org/D37663 ___

[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path

klimek added inline comments. Comment at: clangd/GlobalCompilationDatabase.cpp:98-100 +if (CDB) + return CDB; +return nullptr; Isn't that the same as "return CDB"? https://reviews.llvm.org/D37150 ___ c

[PATCH] D37634: clang-rename: let -force handle multiple renames

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. LG https://reviews.llvm.org/D37634 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-com

[PATCH] D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action

klimek accepted this revision. klimek added a comment. LG. Nice, thanks! Comment at: lib/Tooling/Refactoring/Rename/RenamingAction.cpp:42 + +class LocalRename : public RefactoringAction { +public: I have to admit, the implementation here is pretty neat! :) Re

[PATCH] D37903: Fix assume-filename handling in clang-format.el

klimek added a reviewer: phst. klimek added a comment. +Philipp, who is our emacs expert :) https://reviews.llvm.org/D37903 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35743: [clang-format] Handle Structured binding declaration in C++17

klimek added a comment. In https://reviews.llvm.org/D35743#841197, @chh wrote: > Daniel, Manuel, I will take over this CL since Yan has finished his > internship at Google., > Yan's latest patch to tryToParseLambda looks acceptable to me. > I think it should take care of new kw_auto in additio

[PATCH] D35743: [clang-format] Adjust space around &/&& of structured bindings

klimek added a comment. So we actually didn't need to change the UnwrappedLineParser? I assume we still incorrectly assume the structured bindings are labmdas then? https://reviews.llvm.org/D35743 ___ cfe-commits mailing list cfe-commits@lists.llvm

[PATCH] D37980: [clang-format] Better parsing of lambda captures with initializer expressions.

klimek added a comment. Thanks for the patch and the discussion around this. I fixed this in r313622 in what I think is a more principled approach that also works for nested lambdas (and gets rid of a lot of now-obsolete code). The big problem with this code was that it evolved a bit to the poin

[PATCH] D33589: clang-format: consider not splitting tokens in optimization

klimek added a comment. I find the current semantics of the functions a bit surprising, specifically: ... reflowProtrudingToken(..., bool Reflow) is really confusing me :) I'd have expected something like this where we currently call breakProtrudingToken(): if (CanBreak) { ReflowPenalty =

[PATCH] D35743: [clang-format] Adjust space around &/&& of structured bindings

klimek added a comment. Ok, we still need to fix structured bindings in the UnwrappedLineParser. Unfortunately isCppStructuredBinding requires a "previous token" function, which we don't really have in the UnwrappedLineParser. /me goes thinking more about that part of the problem. That should b

[PATCH] D33589: clang-format: consider not splitting tokens in optimization

klimek added a comment. In https://reviews.llvm.org/D33589#876196, @Typz wrote: > This cannot be implemented where we currently call breakProtrudingToken(), > since this function starts by 'creating' the BreakableToken and dealing with > meany corner cases: so this should be done before in any

[PATCH] D37813: clang-format: better handle namespace macros

klimek added a comment. I think instead of introducing more and more special cases of macros we might want to handle, we should instead allow specifying macro productions globally. https://reviews.llvm.org/D37813 ___ cfe-commits mailing list cfe-co

[PATCH] D35743: [clang-format] Adjust space around &/&& of structured bindings

klimek added a comment. 1. Can you rebase after r313742? I fixed detection of structured bindings in the UnwrappedLineParser, that might affect this patch 2. Isn't LLVM style the reverse? That is, shouldn't that be LLVM style: auto &&[a, b] = ... Pointer/Ref-to-type-style: auto&& [a,

[PATCH] D38032: [clangd] Serialize onDiagnosticsReady callbacks for the same file.

klimek added inline comments. Comment at: clangd/ClangdServer.cpp:321-324 +// FIXME(ibiryukov): get rid of '<' comparison here. In the current +// implementation diagnostics will not be reported after version counters' +// overflow. This should not happen in practice,

[PATCH] D38032: [clangd] Serialize onDiagnosticsReady callbacks for the same file.

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. LG Comment at: clangd/ClangdServer.h:287 + std::mutex DiagnosticsMutex; + llvm::StringMap ReportedDiagnosticVersions; }; Comment what it maps from.

[PATCH] D35743: [clang-format] Adjust space around &/&& of structured bindings

klimek added a comment. Apart from nits, LGTM, unless Daniel sees issues. Comment at: lib/Format/TokenAnnotator.cpp:2516 return Style.SpacesInAngles; + // space before TT_StructuredBindingLSquare + if (Right.is(TT_StructuredBindingLSquare)) Super-Nit: ca

[PATCH] D37554: [libclang] Allow crash recovery with LIBCLANG_NOTHREADS

klimek added a reviewer: akyrtzi. klimek added a comment. Adding Argyrios, who might have insight on how this is used. I think this had the wrong list of reviewers so far :( https://reviews.llvm.org/D37554 ___ cfe-commits mailing list cfe-commits@li

[PATCH] D38151: [clang] Fix isExternC matcher docs

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. LG Repository: rL LLVM https://reviews.llvm.org/D38151 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/m

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

klimek added a comment. The run on llvm indicates that we don't want this to trigger if the base class doesn't have anything to copy (that is, no fields & a defaulted copy-ctor, or an empty copy-ctor). https://reviews.llvm.org/D33722 ___ cfe-commi

[PATCH] D37813: clang-format: better handle namespace macros

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. LG Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D37813/new/ https://reviews.llvm.org/D37813 ___ cfe-commits mail

[PATCH] D65092: [clang] Add isDirectlyDerivedFrom AST Matcher.

klimek added inline comments. Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2637 internal::Matcher, Base) { - return Finder->classIsDerivedFrom(&Node, Base, Builder); + return Finder->classIsDerivedFrom(&Node, Base, Builder, false); } ---

[PATCH] D65092: [clang] Add isDirectlyDerivedFrom AST Matcher.

klimek requested changes to this revision. klimek added inline comments. This revision now requires changes to proceed. Comment at: clang/docs/LibASTMatchersReference.html:5277 +MatcherCXXRecordDecl>isDirectlyDe

[PATCH] D65125: clang-format: Fix namespace end comments for namespaces with attributes and macros

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. LG, thx! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65125/new/ https://reviews.llvm.org/D65125 ___ cfe-commits mailing list cfe-commi

[PATCH] D65092: [clang] Add isDirectlyDerivedFrom AST Matcher.

klimek added a comment. So, I did like the more exhaustive doc, I thought you'd move it to the code so it also shows up in the generated doc page :) (sorry for not being clearer here) Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2637 internal::Match

[PATCH] D65227: clang-format: Support `if CONSTEXPR` if CONSTEXPR is a macro.

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. I am surprised this works as well as it does, but tests seem to indicate it does, so... LG (minus potentially reducing some duplication :) Comment at: clang/lib/Format/Token

[PATCH] D66462: Removed the 'id' AST matcher, which is superseded by '.bind()'

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. Yay, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66462/new/ https://reviews.llvm.org/D66462 ___

[PATCH] D63734: Update CODE_OWNERS.txt for clang-doc

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. LG CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63734/new/ https://reviews.llvm.org/D63734 ___ cfe-commits mailing list cfe-commits@lis

[PATCH] D37813: clang-format: better handle namespace macros

klimek added a comment. In D37813#1254891 , @Typz wrote: > In D37813#1254865 , @klimek wrote: > > > In D37813#1253813 , @Typz wrote: > > > > > In D37813#1184051

[PATCH] D37813: clang-format: better handle namespace macros

klimek added a comment. In D37813#1512317 , @Typz wrote: > The patch goal is indeed to indent the content of namespace-macro blocks like > the content of any 'regular' namespace. So it should look like the content of > a namespace, possibly depending on

[PATCH] D37813: clang-format: better handle namespace macros

klimek added inline comments. Comment at: unittests/Format/NamespaceEndCommentsFixerTest.cpp:582 + EXPECT_EQ("TESTSUITE() {\n" +" int i;\n" +"} // TESTSUITE()", All of the fixNamespaceEndComments tests are indented, but standard llvm sty

[PATCH] D37813: clang-format: better handle namespace macros

klimek added inline comments. Comment at: unittests/Format/NamespaceEndCommentsFixerTest.cpp:526 EXPECT_EQ("namespace A {\n" " int i;\n" "} // namespace A", Typz wrote: > Should I also fix these tests? > > They already existed befor

[PATCH] D56090: Add a matcher for members of an initializer list expression

klimek added inline comments. Comment at: include/clang/ASTMatchers/ASTMatchers.h:3527 + return N < Node.getNumInits() && + InnerMatcher.matches(*Node.getInit(N)->IgnoreParenImpCasts(), Finder, + Builder); aaron.ballman wro

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

klimek added a comment. In D56444#1350746 , @sammccall wrote: > In D56444#1350252 , @JonasToth wrote: > > > I still see the unit-test crashing for `ExprMutAnalyzer` (just apply the > > last two tests > > https://g

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

klimek added a comment. In D56444#1351056 , @aaron.ballman wrote: > In D56444#1350849 , @JonasToth wrote: > > > In D56444#1350746 , @sammccall > > wrote: > > > > > @klimek:

[PATCH] D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on.

klimek added a comment. Just in general, I'd like to add that my experience over the years dealing with folks trying to do AST matchers is that the inability to express something is much more of a problem than matching too much, simply because the test cases people think of are usually small, a

[PATCH] D67405: Make FormatToken::Type private.

klimek created this revision. klimek added a reviewer: sammccall. Herald added a project: clang. This enables us to intercept changes to the token type via setType(), which is a precondition for being able to use multi-pass formatting for macro arguments. Repository: rG LLVM Github Monorepo h

[PATCH] D67541: [ClangFormat] Future-proof Standard option, allow floating or pinning to arbitrary lang version

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. LG Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67541/new/ https://reviews.llvm.org/D67541 ___ c

[PATCH] D67670: [clang-format][PR41964] Fix crash with SIGFPE when TabWidth is set to 0 and line starts with tab

klimek added inline comments. Comment at: clang/lib/Format/FormatTokenLexer.cpp:660 case '\t': -Column += Style.TabWidth - Column % Style.TabWidth; +Column += Style.TabWidth - (Column ? Column % Style.TabWidth : 0); break; Shouldn'

[PATCH] D67670: [clang-format][PR41964] Fix crash with SIGFPE when TabWidth is set to 0 and line starts with tab

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. LG. That makes lots of sense now :) Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67670/new/ https://reviews.llvm.org/D67670 ___

[PATCH] D67718: [clang-format][PR41899] PointerAlignment: Left leads to useless space in lambda intializer expression

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. LG Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67718/new/ https://reviews.llvm.org/D67718 ___ cfe-commits mail

[PATCH] D67949: [clang-format] [PR36858] Add missing .hh and .cs extensions from python support utilities

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. lg Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67949/new/ https://reviews.llvm.org/D67949 ___ cfe-commits mail

[PATCH] D68072: Reference qualifiers in member templates causing extra indentation.

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. LG, nice patch, thanks! Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68072/new/ https://reviews.llvm.org/D68072 _

[PATCH] D68296: clang-format: Add ability to wrap braces after multi-line control statements

klimek added a comment. In D68296#1691393 , @mitchell-stellar wrote: > Thanks for the review. I have added documentation updates. > > I do not have a public style guide to reference. My company just switched to > auto-clang-formatting all of our code, an

[PATCH] D38615: [libclang] Only mark CXCursors for explicit attributes with a type

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. lg https://reviews.llvm.org/D38615 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-com

[PATCH] D45726: Format closing braces when reformatting the line containing theopening brace.This required a couple of yaks to be shaved:1. MatchingOpeningBlockLineIndex was misused to also store the

klimek created this revision. klimek added a reviewer: krasimir. ...doesn't work correctly for "} else {". 2. We needed to change the API of AffectedRangeManager to not use iterators; we always passed in begin / end for the whole container before, so there was no mismatch in generality. 3. W

[PATCH] D28462: clang-format: Add new style option AlignConsecutiveMacros

klimek added inline comments. Comment at: lib/Format/WhitespaceManager.cpp:438 + + AlignTokens(Style, + [&](const Change &C) { I'm not sure whether we should use AlignTokens here, given that we pass in a parameter to basically skip all its interest

[PATCH] D45726: Format closing braces when reformatting the line containing theopening brace.

klimek added a comment. In https://reviews.llvm.org/D45726#1071925, @krasimir wrote: > Another point: for the example in the summary about bailing-out early, is > there a test for this already? If not, we should add one. Yep, there already is one - I regressed that with my change at first ;)

[PATCH] D45726: Format closing braces when reformatting the line containing theopening brace.

klimek updated this revision to Diff 143274. klimek marked 2 inline comments as done. klimek added a comment. Address comments. Repository: rC Clang https://reviews.llvm.org/D45726 Files: lib/Format/AffectedRangeManager.cpp lib/Format/AffectedRangeManager.h lib/Format/Format.cpp lib/

[PATCH] D45726: Format closing braces when reformatting the line containing theopening brace.

This revision was automatically updated to reflect the committed changes. Closed by commit rL330573: Format closing braces when reformatting the line containing the opening brace. (authored by klimek, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.

[PATCH] D45719: [clang-Format] Fix indentation of member call after block

klimek added inline comments. Comment at: unittests/Format/FormatTest.cpp:4359 + "return 3;\n" + " }).as("");\n" + "}"); What would be interesting is tests that: a) have another value after the closing }; doesn't rea

[PATCH] D46062: [clang-format] Start formatting cpp code in raw strings in google style

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. LG Repository: rC Clang https://reviews.llvm.org/D46062 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/

[PATCH] D44609: [Clang-Format] New option BreakBeforeLambdaBody to manage lambda line break inside function parameter call

klimek added inline comments. Comment at: include/clang/Format/Format.h:891 + /// \endcode + bool BreakBeforeLambdaBody; + I'd just make that default for Allman style. Comment at: lib/Format/TokenAnnotator.cpp:2844 if (isAllmanBrace(Left)

[PATCH] D46024: [clang-format] Add SpaceBeforeCpp11BracedList option.

klimek added a comment. Is this written down somewhere? https://webkit.org/code-style-guidelines/ doesn't seem to mention it. Repository: rC Clang https://reviews.llvm.org/D46024 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lis

[PATCH] D36610: [Tooling] Add option to getFullyQualifiedName using a custom PritingPolicy

klimek added a comment. I believe this was accepted by rnk - are you waiting for specific further feedback? https://reviews.llvm.org/D36610 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-c

[PATCH] D45719: [clang-Format] Fix indentation of member call after block

klimek added inline comments. Comment at: unittests/Format/FormatTest.cpp:4359 + "return 3;\n" + " }).as("");\n" + "}"); ank wrote: > klimek wrote: > > ank wrote: > > > klimek wrote: > > > > What would be interesting

[PATCH] D45719: [clang-Format] Fix indentation of member call after block

klimek added inline comments. Comment at: unittests/Format/FormatTest.cpp:4449-4450 + " })\n" + " .foo(\"aaa\"\n" + " \"bb\");\n" +

[PATCH] D45719: [clang-Format] Fix indentation of member call after block

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. LG. Repository: rC Clang https://reviews.llvm.org/D45719 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin

[PATCH] D44609: [Clang-Format] New option BreakBeforeLambdaBody to manage lambda line break inside function parameter call

klimek added a comment. In https://reviews.llvm.org/D44609#1143895, @Wawha wrote: > Hello, > > after my last modification (require by previous comment), I do not see any > feedback or validation for this patch. > Is their something special to do in order to go forward on this patch? > > Lambd

[PATCH] D48159: [clangd] Implement hover for "auto" and "decltype"

klimek added inline comments. Comment at: clangd/XRefs.cpp:559 + //- auto& i = 1; + bool VisitDeclaratorDecl(DeclaratorDecl *D) { +if (!D->getTypeSourceInfo() || sammccall wrote: > malaperle wrote: > > sammccall wrote: > > > out of curiosity, why not implem

[PATCH] D28462: clang-format: Add new style option AlignConsecutiveMacros

klimek added a comment. In https://reviews.llvm.org/D28462#1147019, @enyquist wrote: > @klimek fair point. To be honest, I've pretty much lost interest / momentum > on this feature, I very much doubt I will ever go back and re-implement from > scratch as you suggest. > Not meaning to sound rud

[PATCH] D48159: [clangd] Implement hover for "auto" and "decltype"

klimek added a reviewer: rsmith. klimek added inline comments. Comment at: clangd/XRefs.cpp:559 + //- auto& i = 1; + bool VisitDeclaratorDecl(DeclaratorDecl *D) { +if (!D->getTypeSourceInfo() || malaperle wrote: > klimek wrote: > > sammccall wrote: > > > ma

[PATCH] D48159: [clangd] Implement hover for "auto" and "decltype"

klimek accepted this revision. klimek added inline comments. Comment at: clangd/XRefs.cpp:559 + //- auto& i = 1; + bool VisitDeclaratorDecl(DeclaratorDecl *D) { +if (!D->getTypeSourceInfo() || klimek wrote: > malaperle wrote: > > klimek wrote: > > > sammcca

[PATCH] D45094: [LibTooling] Make interface of VFS injection into ClangTool more user-friendly

klimek added inline comments. Comment at: lib/Basic/VirtualFileSystem.cpp:328 void OverlayFileSystem::pushOverlay(IntrusiveRefCntPtr FS) { + // FIXME: OverlayFS containing another one in its stack could be flattened. FSList.push_back(FS); ilya-biryukov wrote

[PATCH] D28462: clang-format: Add new style option AlignConsecutiveMacros

klimek added a comment. In https://reviews.llvm.org/D28462#1148732, @enyquist wrote: > @klimek having gotten that out of the way, I do occasionally drink too much > and have sudden urges to re-implement things from scratch. Close it if you > need to, since I can't commit to anything, but it

[PATCH] D44609: [Clang-Format] New option BreakBeforeLambdaBody to manage lambda line break inside function parameter call

klimek added inline comments. Herald added a subscriber: acoomans. Comment at: lib/Format/TokenAnnotator.cpp:627 } +if(Style.BraceWrapping.BeforeLambdaBody && Current->is(TT_LambdaLSquare)) { +++Left->BlockParameterCount; Why do we want to increa

[PATCH] D44609: [Clang-Format] New option BreakBeforeLambdaBody to manage lambda line break inside function parameter call

klimek added inline comments. Comment at: lib/Format/TokenAnnotator.cpp:627 } +if(Style.BraceWrapping.BeforeLambdaBody && Current->is(TT_LambdaLSquare)) { +++Left->BlockParameterCount; Wawha wrote: > klimek wrote: > > Why do we want to increase t

[PATCH] D44609: [Clang-Format] New option BeforeLambdaBody to manage lambda line break inside function parameter call (in Allman style)

klimek added inline comments. Comment at: lib/Format/ContinuationIndenter.cpp:1307 + (Style.BraceWrapping.BeforeLambdaBody && Current.Next != nullptr && +Current.Next->is(TT_LambdaLSquare))); State.Stack.back().IsInsideObjCArrayLiteral = I think

[PATCH] D68554: [clang-format] Proposal for clang-format to give compiler style warnings

klimek added a comment. Why not build a tool that takes the diff output of clang-format and changes it to what you want? Wouldn't that be a couple of lines of python? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68554/new/ https://reviews.llvm.org/D68554 ___

[PATCH] D68660: [tooling] Teach Tooling to understand compilation with offloading.

klimek added inline comments. Comment at: clang/lib/Tooling/Tooling.cpp:117 // The one job we find should be to invoke clang again. const auto &Cmd = cast(*Jobs.begin()); if (StringRef(Cmd.getCreator().getName()) != "clang") { tra wrote: > hliao wrote: >

<    1   2   3   4   5   6   >