[PATCH] D40068: Implement more accurate penalty & trade-offs while breaking protruding tokens.

klimek created this revision. This is a counter-proposal to https://reviews.llvm.org/D33589. https://reviews.llvm.org/D40068 Files: lib/Format/BreakableToken.cpp lib/Format/BreakableToken.h lib/Format/ContinuationIndenter.cpp unittests/Format/FormatTest.cpp

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

klimek added a comment. One interesting trade-off I'm running into: My gut feeling is that we really want to make local decisions about whether we want to break/reflow - this makes the code significantly simpler (IMO), and handles all tests in this patch correctly, but is fundamentally limiting

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

klimek added a comment. In https://reviews.llvm.org/D33589#920160, @Typz wrote: > ping ? I'm working on understanding this better :) I've refactored the code a bit so I could fully understand the problem, which I now do (sorry for this taking a while, but it took me multiple hours to work

[PATCH] D39900: Refactor ContinuationIndenter's breakProtrudingToken logic into slightly moreorthogonal pieces.

This revision was automatically updated to reflect the committed changes. Closed by commit rL318141: Refactor ContinuationIndenter's breakProtrudingToken logic. (authored by klimek). Repository: rL LLVM https://reviews.llvm.org/D39900 Files: cfe/trunk/lib/Format/BreakableToken.cpp

[PATCH] D39900: Refactor ContinuationIndenter's breakProtrudingToken logic into slightly moreorthogonal pieces.

klimek added a comment. In https://reviews.llvm.org/D39900#921885, @krasimir wrote: > Maybe we should further refactor `getRawStringStyle` into > `llvm::Optional> getRawStringStyleAndDelimiter` > and that would nicely take care of the duplicated effort? I thought

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

klimek added a comment. In https://reviews.llvm.org/D32478#920345, @Typz wrote: > > Unless I'm missing something, I'd agree with Daniel; this is not a rule > > that's widely used, and I'd say reformatting a code base to the > > clang-formatted variant will not regress readability. > >

[PATCH] D39900: Refactor ContinuationIndenter's breakProtrudingToken logic into slightly moreorthogonal pieces.

klimek added inline comments. Comment at: lib/Format/ContinuationIndenter.cpp:1028 + unsigned Penalty = + handleEndOfLine(Current, State, DryRun, AllowBreak); krasimir wrote: > Why `handleEndOfLine`? Is it guaranteed that here we've reached the end of >

[PATCH] D39900: Refactor ContinuationIndenter's breakProtrudingToken logic into slightly moreorthogonal pieces.

klimek updated this revision to Diff 122433. klimek marked an inline comment as done. klimek added a comment. - Add test. Updating D39900: Refactor ContinuationIndenter's breakProtrudingToken logic into slightly more

[PATCH] D39842: Allow to store precompiled preambles in memory.

klimek added inline comments. Comment at: include/clang/Frontend/PrecompiledPreamble.h:99 /// Changes options inside \p CI to use PCH from this preamble. Also remaps - /// main file to \p MainFileBuffer. + /// main file to \p MainFileBuffer and updates \p VFS to ensure

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

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. LG. https://reviews.llvm.org/D37554 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D39900: Refactor ContinuationIndenter's breakProtrudingToken logic into slightly moreorthogonal pieces.

klimek created this revision. This patch allowed me to experiment with various alternatives to https://reviews.llvm.org/D33589. https://reviews.llvm.org/D39900 Files: lib/Format/BreakableToken.cpp lib/Format/ContinuationIndenter.cpp lib/Format/ContinuationIndenter.h

[PATCH] D39836: [clangd] Drop impossible completions (unavailable or inaccessible)

klimek added a comment. In https://reviews.llvm.org/D39836#920313, @sammccall wrote: > In https://reviews.llvm.org/D39836#920306, @ilya-biryukov wrote: > > > I personally think they're useful to have anyway, and they don't add much > > noise when they're at the end of completions list. > > I

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

klimek added a comment. In https://reviews.llvm.org/D32478#920275, @Typz wrote: > > Sorry for the long response time. I don't see this style rule expressed > > explicitly in the Skia or QtCreator style guides - is this something that > > just happens to be done sometimes in the code bases? > >

[PATCH] D39799: [Tooling] Use FixedCompilationDatabase when `compile_flags.txt` is found.

klimek accepted this revision. klimek added a comment. lg Comment at: lib/Tooling/CompilationDatabase.cpp:312 +FixedCompilationDatabase::loadFromFile(StringRef Path, std::string ) { + ErrorMsg.clear(); + llvm::ErrorOr File = sammccall wrote:

[PATCH] D32478: [clang-format] Fix AlignOperands when BreakBeforeBinaryOperators is set

klimek added a comment. In https://reviews.llvm.org/D32478#806757, @Typz wrote: > I renamed the option to `AlignAfterOperator`, is it acceptable now? > (I also thought of `UnindentOperator`, but I think it is better to keep the > notion of alignment) > > Note that this style is used in

[PATCH] D39799: [Tooling] Use FixedCompilationDatabase when `compile_flags.txt` is found.

klimek added inline comments. Comment at: include/clang/Tooling/CompilationDatabase.h:188 + static std::unique_ptr + loadFromFile(StringRef Path, std::string ); + Perhaps loadFromTextFile? Comment at: lib/Tooling/CompilationDatabase.cpp:312

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

klimek added a comment. Sorry for jumping in late, but I'm somewhat questioning whether an extra function in the API is worth the cost here: If you already have a printing policy, getting the type name will be 1 line instead of 2; having an extra function in a large API seems not worth it at a

[PATCH] D5767: Template Instantiation Observer + a few other templight-related changes

klimek added a comment. Weekly ping. This week is C++ committee, so it's again going to be hard to get Richards attention :( It's an unfortunate time of year to land major API-touching changes. https://reviews.llvm.org/D5767 ___ cfe-commits

[PATCH] D34272: [Tooling] A new framework for executing clang frontend actions.

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. lg https://reviews.llvm.org/D34272 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D5767: Template Instantiation Observer + a few other templight-related changes

klimek added a comment. Just FYI, I talked with Richard and he'll not get to it before next week. Hope that's fine. https://reviews.llvm.org/D5767 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D34272: [Tooling] A new framework for executing clang frontend actions.

klimek added inline comments. Comment at: include/clang/Tooling/Execution.h:76 + + void appendArgumentsAdjuster(ArgumentsAdjuster Adjuster); + I think the argument adjuster adjustment shouldn't be part of this interface, as the argument adjusters cannot be

[PATCH] D5767: Template Instantiation Observer + a few other templight-related changes

klimek added a comment. Whee, thanks for driving this, much appreciated :D Comment at: include/clang/FrontendTool/Utils.h:25 + +std::unique_ptr CreateFrontendAction(CompilerInstance ); Please add a comment now that this is exported.

[PATCH] D34272: [Tooling] A new framework for executing clang frontend actions.

klimek added inline comments. Comment at: include/clang/Tooling/CommonOptionsParser.h:90 /// - /// This constructor exits program in case of error. + /// If \p ExitOnError is set (default), This constructor exits program in case + /// of error; otherwise, this sets the

[PATCH] D38583: [clangd] Added async API to run code completion.

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. LG Comment at: clangd/ClangdServer.h:236-237 + /// + /// Request is processed asynchronously, you can use returned future to wait + /// for results of async request. +

[PATCH] D38583: [clangd] Added async API to run code completion.

klimek added inline comments. Comment at: clangd/ClangdServer.cpp:225 + // A task that will be run asynchronously. + PackagedTask Task([=]() mutable { // 'mutable' to reassign Preamble variable. +if (!Preamble) { ilya-biryukov wrote: > klimek wrote: > > It

[PATCH] D38583: [clangd] Added async API to run code completion.

klimek added inline comments. Comment at: clangd/ClangdServer.cpp:222 + + std::shared_ptr Preamble = + Resources->getPossiblyStalePreamble(); I think we use "const type" everywhere. Comment at: clangd/ClangdServer.cpp:225 + // A task

[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 ___

[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

[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

[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:

[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] 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

[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] 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

[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] 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

[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] 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

[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

[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

[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] 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! :)

[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

[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 ___

[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] 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

[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

[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

[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:

[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:

[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

[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

[PATCH] D36397: [clangd] Fixed a data race.

klimek added inline comments. Comment at: clangd/ClangdUnit.cpp:714 -void CppFile::cancelRebuilds() { +void CppFile::cancelRebuild() { deferCancelRebuild().get(); } + Somewhat orthogonal question - why CppFile? We do support other languages in Clangd, right?

[PATCH] D35955: clang-format: Add preprocessor directive indentation

klimek added a comment. LG from my side. https://reviews.llvm.org/D35955 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36398: [clangd] Check if CompileCommand has changed on forceReparse.

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. lg Comment at: unittests/clangd/ClangdTests.cpp:509-510 + /*RunSynchronously=*/true); + // No need to sync reparses, because RunSynchronously is set +

[PATCH] D36261: [clangd] Use multiple working threads in clangd.

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. lg https://reviews.llvm.org/D36261 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D36529: Fixed a race condition in PrecompiledPreamble.

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. LG, as discussed in person, it's probably a good idea to try to get rid of the non-file-creating version, if possible, or at least fix the comments on the functions to make this behavior

[PATCH] D36529: Fixed a race condition in PrecompiledPreamble.

klimek added inline comments. Comment at: lib/Frontend/PrecompiledPreamble.cpp:470-471 + int FD; + auto EC = llvm::sys::fs::createTemporaryFile(Prefix, Suffix, /*ref*/ FD, + /*ref*/ File); if (EC) I don't

[PATCH] D35355: Fix templated type alias completion when using global completion cache

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. LG https://reviews.llvm.org/D35355 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D35955: clang-format: Add preprocessor directive indentation

klimek added a comment. In https://reviews.llvm.org/D35955#835994, @euhlmann wrote: > In https://reviews.llvm.org/D35955#835439, @klimek wrote: > > > I think if we need this info, we can just make it count down to -1 again > > (or, but that's isomorphic, let it run from 0 and make sure we never

[PATCH] D35955: clang-format: Add preprocessor directive indentation

klimek added a comment. In https://reviews.llvm.org/D35955#834914, @djasper wrote: > Manuel: Can you take a look at the last comment here? Why does PPBranchLevel > start at -1? It's a perhaps too-clever implementation to make sure that we can have a per-branch data structure (indexed from 0)

[PATCH] D36398: [clangd] Check if CompileCommand has changed on forceReparse.

klimek added a comment. Also missing tests :) Comment at: clangd/ClangdUnitStore.cpp:45 + .first; +Result.RemovedFile = nullptr; + } else if (!compileCommandsAreEqual(It->second->getCompileCommand(), ilya-biryukov wrote: > klimek wrote: > >

[PATCH] D36261: [clangd] Use multiple working threads in clangd.

klimek added a comment. High-level question: Why can't we use llvm::ThreadPool? https://reviews.llvm.org/D36261 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36159: clang-format: [JS] handle single lines comments ending in `\\`.

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. lg Comment at: lib/Format/FormatTokenLexer.cpp:544-545 +while (BackslashPos != StringRef::npos) { + if (BackslashPos + 1 < FormatTok->TokenText.size() && +

[PATCH] D36397: [clangd] Fixed a data race.

klimek added a comment. In https://reviews.llvm.org/D36397#833890, @ilya-biryukov wrote: > In https://reviews.llvm.org/D36397#833883, @klimek wrote: > > > Tests? > > > TSAN does not catch this (as it's a logical error) and it requires a rather > bizarre timing of file reparses to trigger. > I

[PATCH] D36397: [clangd] Fixed a data race.

klimek added a comment. Tests? https://reviews.llvm.org/D36397 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36398: [clangd] Check if CompileCommand has changed on forceReparse.

klimek added inline comments. Comment at: clangd/ClangdUnitStore.cpp:45 + .first; +Result.RemovedFile = nullptr; + } else if (!compileCommandsAreEqual(It->second->getCompileCommand(), Just say RemovedFile = nullptr in the struct?

[PATCH] D35355: Fix templated type alias completion when using global completion cache

klimek added a comment. Sorry for missing this - can you add a test? https://reviews.llvm.org/D35355 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36308: Add handling for DeducedType to HasDeclarationMatcher

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. Thanks! LG after comment change. Also, should we add some tests to clang-tidy? :) Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:745 + +// For deduced

[PATCH] D36261: [clangd] Use multiple working threads in clangd.

klimek added inline comments. Comment at: clangd/ClangdServer.h:105 +/// A helper class to pass concurrency parameters to ClangdScheduler. +class SchedulingParams { +public: I think calling it "Options" is more idiomatic. Comment at:

[PATCH] D36154: Adapt clang-tidy checks to changing semantics of hasDeclaration.

This revision was automatically updated to reflect the committed changes. Closed by commit rL309810: Adapt clang-tidy checks to changing semantics of hasDeclaration. (authored by klimek). Repository: rL LLVM https://reviews.llvm.org/D36154 Files:

[PATCH] D36184: [clang-diff] Filter AST nodes

klimek added a comment. Just as a general note: adding cfe-commits after the fact is usually not a good idea, as we lose the history of the review in the email list (which is the source of truth). https://reviews.llvm.org/D36184 ___ cfe-commits

[PATCH] D27104: Unify and simplify the behavior of the hasDeclaration matcher.

This revision was automatically updated to reflect the committed changes. Closed by commit rL309809: Unify and simplify the behavior of the hasDeclaration matcher. (authored by klimek). Changed prior to commit: https://reviews.llvm.org/D27104?vs=108608=109325#toc Repository: rL LLVM

[PATCH] D36156: [rename] Introduce symbol occurrences

klimek added inline comments. Comment at: include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h:32 +/// a macro expansion. +class SymbolOccurrence { +public: I understand the exact vs. non-exact idea, but can you expand a bit on how Obj-C code looks

[PATCH] D36154: Adapt clang-tidy checks to changing semantics of hasDeclaration.

klimek added a comment. ptal https://reviews.llvm.org/D36154 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36154: Adapt clang-tidy checks to changing semantics of hasDeclaration.

klimek updated this revision to Diff 109287. klimek marked an inline comment as done. klimek added a comment. Address review comment. https://reviews.llvm.org/D36154 Files: clang-tidy/google/StringReferenceMemberCheck.cpp clang-tidy/misc/DanglingHandleCheck.cpp

[PATCH] D36155: Use VFS operations in FileManager::makeAbsolutePath.

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. Nice catch! lg https://reviews.llvm.org/D36155 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D36154: Adapt clang-tidy checks to changing semantics of hasDeclaration.

klimek created this revision. Herald added a subscriber: JDevlieghere. https://reviews.llvm.org/D36154 Files: clang-tidy/google/StringReferenceMemberCheck.cpp clang-tidy/misc/DanglingHandleCheck.cpp clang-tidy/misc/InaccurateEraseCheck.cpp clang-tidy/misc/UseAfterMoveCheck.cpp

[PATCH] D35012: [refactor] Add the AST source selection component

klimek added a comment. Apart from nits, looks OK to me - Eric, are all your concerns addressed? Comment at: include/clang/Tooling/Refactoring/ASTSelection.h:51-53 + ast_type_traits::DynTypedNode Node; + SourceSelectionKind SelectionKind; + std::vector Children;

[PATCH] D27104: Unify and simplify the behavior of the hasDeclaration matcher.

klimek added a reviewer: bkramer. klimek added a comment. Adding Ben as reviewer. https://reviews.llvm.org/D27104 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27104: Unify and simplify the behavior of the hasDeclaration matcher.

klimek updated this revision to Diff 108608. klimek added a comment. Update to handle hasDeclaration for type alias template decls. https://reviews.llvm.org/D27104 Files: include/clang/ASTMatchers/ASTMatchers.h include/clang/ASTMatchers/ASTMatchersInternal.h

[PATCH] D35828: Fix incorrect use of current directory to find moved paths in ASTReader.

This revision was automatically updated to reflect the committed changes. Closed by commit rL308962: Fix incorrect use of current directory to find moved paths in ASTReader. (authored by klimek). Repository: rL LLVM https://reviews.llvm.org/D35828 Files:

[PATCH] D35828: Fix incorrect use of current directory to find moved paths in ASTReader.

klimek created this revision. CurrentDir was set as the path of the current module, but that can change as part of a chain of loaded modules. When we try to locate a file mentioned in a module that does not exist, we use a heuristic to look at the relative path between the original location of

[PATCH] D35794: [clang-format] Fix comment levels between '} else {' and PPDirective.

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. lg Comment at: lib/Format/UnwrappedLineParser.h:126 bool eof() const; - void nextToken(); + // LevelDifference is the difference of levels after and before this token.

[PATCH] D35783: Ignore shadowing for declarations coming from within macros.

klimek added a comment. Could we perhaps only suppress the warning when we shadow a variable within the same declcontext? Generally, with macros, shadowing local variables is more surprising. https://reviews.llvm.org/D35783 ___ cfe-commits

[PATCH] D35783: Ignore shadowing for declarations coming from within macros.

klimek added a comment. Your patch description sounds like you're only switching the warning off when the scope ends in a macro, but the patch looks different. For example: #define M int x int x; void f() { M; x = 42; // the user probably meant ::x here } would also be

[PATCH] D34440: [Clang] Expand response files before loading compilation database

klimek added a comment. In https://reviews.llvm.org/D34440#812938, @vladimir.plyashkun wrote: > > If you want one unified format, the compilation database is it. > > Is the `clang-tidy ... -- ` meant to be more or less a drop-in > replacement for `clang ` (arguments-wise)? > If yes, this

[PATCH] D35012: [refactor] Add the AST source selection component

klimek added inline comments. Comment at: lib/Tooling/Refactoring/ASTSelection.cpp:164 + unsigned NumMatches = 0; + for (Decl *D : Context.getTranslationUnitDecl()->decls()) { +if (ObjCImplEndLoc.isValid() && arphaman wrote: > klimek wrote: > > arphaman

[PATCH] D35012: [refactor] Add the AST source selection component

klimek added inline comments. Comment at: lib/Tooling/Refactoring/ASTSelection.cpp:164 + unsigned NumMatches = 0; + for (Decl *D : Context.getTranslationUnitDecl()->decls()) { +if (ObjCImplEndLoc.isValid() && arphaman wrote: > klimek wrote: > > Why don't

[PATCH] D33644: Add default values for function parameter chunks

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. lg Comment at: lib/Sema/SemaCodeComplete.cpp:2422 + std::string DefValue{srcText}; + // TODO: remove this check if the Lexer::getSourceText value is fixed and + // this

[PATCH] D33644: Add default values for function parameter chunks

klimek added a comment. In https://reviews.llvm.org/D33644#812693, @yvvan wrote: > So do we wait until the '=' case is more clear? This change should not break > anything if it's fixed in either direction (if '=' will be provided always or > never) Ok, if you add a TODO I think this is fine

[PATCH] D35012: [refactor] Add the AST source selection component

klimek added inline comments. Comment at: lib/Tooling/Refactoring/ASTSelection.cpp:100 + SelectionStack.back().Children.push_back(std::move(Node)); +return true; + } arphaman wrote: > klimek wrote: > > Why do we always stop traversal? > False stops

[PATCH] D35012: [refactor] Add the AST source selection component

klimek added inline comments. Comment at: include/clang/Tooling/Refactoring/ASTSelection.h:30 + /// inside of its source range. + ContainsSelectionPoint, + It's unclear what a selection point is. I'd have expected this to mean "overlaps" when first reading

[PATCH] D34440: [Clang] Expand response files before loading compilation database

klimek added a comment. In https://reviews.llvm.org/D34440#809581, @vladimir.plyashkun wrote: > > Are there any concerns using the alternative? > > I can't say that it's a big problems, but i think that: > //CompilationDatabase.json// is more //CMake //specific format. > It can be generated

[PATCH] D34440: [Clang] Expand response files before loading compilation database

klimek added a comment. In https://reviews.llvm.org/D34440#809325, @vladimir.plyashkun wrote: > Even if i'll change content of //arguments.rsp// to > `-std=c++11 -Ipath/to/include -Ipath/to/include2 -DMACRO ` > and will try to call clang-tidy process in this way: > `clang-tidy -checks=*

[PATCH] D34440: [Clang] Expand response files before loading compilation database

klimek added a comment. In https://reviews.llvm.org/D34440#808156, @vladimir.plyashkun wrote: > **To discuss:** > This patch is required for correct integration //Clang-Tidy// with //CLion > IDE//. > By this moment, we unable to use //CompilationDatabase.json// from //CLion > //side which is

[PATCH] D34267: do more processing in clang-fuzzer (use EmitAssemblyAction)

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. lg https://reviews.llvm.org/D34267 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D34949: [refactor][rename] Use a single base class for class that finds a declaration at location and for class that searches for all occurrences of a specific declaration

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/D34949 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D33644: Add default values for function parameter chunks

klimek added a reviewer: rsmith. klimek added a comment. +Richard, as apparently we get the source ranges for default values of built-in types without the preceding "=", but for user-defined types including the preceding "=" - is that intentional? Comment at:

[PATCH] D34512: Add preliminary Cross Translation Unit support library

klimek added a comment. Yep, I want Richard's approval for the name. I think he already expressed a pro-pulling-out stance. https://reviews.llvm.org/D34512 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

<    1   2   3   4   5   6   >