[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

[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] 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] 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] 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] 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:164 + unsigned NumMatches = 0; + for (Decl *D : Context.getTranslationUnitDecl()->decls()) { +if (ObjCImplEndLoc.isValid() && arphaman wrote: > klimek wrote: > > Why don't

[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] 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] 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] 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] 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] 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] 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] D34304: Allow CompilerInvocations to generate .d files.

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. LG, thx for bearing with me, this looks great. (and sorry if I didn't send this earlier, just noticed I forgot to hit submit :( ) Comment at:

[PATCH] D34696: [refactor] Move clang-rename to Clang

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

[PATCH] D34512: [libTooling] Add preliminary Cross Translation Unit support for libTooling

klimek added a comment. In https://reviews.llvm.org/D34512#800490, @xazax.hun wrote: > It looks like Richard approved libTooling as a dependency for clang on the > mailing list (http://lists.llvm.org/pipermail/cfe-dev/2017-July/054536.html). > If it is ok to have this code in libTooling (for

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

klimek added a reviewer: rsmith. klimek added a comment. +Richard as top-level code owner for new libs. For bikeshedding the name: I'd have liked libIndex, but that's already taken. CrossTU doesn't seem too bad to me, too, though. https://reviews.llvm.org/D34512

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

klimek added a comment. In https://reviews.llvm.org/D34512#800626, @whisperity wrote: > In https://reviews.llvm.org/D34512#800502, @xazax.hun wrote: > > > In https://reviews.llvm.org/D34512#800499, @klimek wrote: > > > > > In https://reviews.llvm.org/D34512#800490, @xazax.hun wrote: > > > > > >

[PATCH] D34687: [Tooling] CompilationDatabase should be able to strip position arguments when `-fsyntax-only` is used

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

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

klimek added a comment. In https://reviews.llvm.org/D33644#793577, @yvvan wrote: > In https://reviews.llvm.org/D33644#793573, @klimek wrote: > > > In https://reviews.llvm.org/D33644#783903, @yvvan wrote: > > > > > Do not evaluate numbers. > > > Check for != "=" is needed not to mess with

[PATCH] D34696: [refactor] Move the core of clang-rename to lib/Tooling/Refactoring

klimek added a comment. In https://reviews.llvm.org/D34696#795020, @arphaman wrote: > In https://reviews.llvm.org/D34696#793613, @klimek wrote: > > > The main thing I'm concerned about is having the main code in core, but > > having all tests in tools-extra. I think if we go that route we

[PATCH] D34696: [refactor] Move the core of clang-rename to lib/Tooling/Refactoring

klimek added a comment. The main thing I'm concerned about is having the main code in core, but having all tests in tools-extra. I think if we go that route we should also move clang-rename and its tests to core. Thoughts? Repository: rL LLVM https://reviews.llvm.org/D34696

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

klimek added a comment. In https://reviews.llvm.org/D33644#793601, @yvvan wrote: > In https://reviews.llvm.org/D33644#793594, @klimek wrote: > > > In https://reviews.llvm.org/D33644#793577, @yvvan wrote: > > > > > In https://reviews.llvm.org/D33644#793573, @klimek wrote: > > > > > > > In

[PATCH] D34755: [clangd] Added a test, checking that gcc install is searched via VFS.

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

[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] 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] 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] 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] 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] 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] 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 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] 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] D33047: [ClangD] Refactor clangd into separate components

klimek added inline comments. Comment at: clangd/ClangdLSPServer.h:23 + +class ClangdLSPServer { + class LSPDiagnosticsConsumer; ilya-biryukov wrote: > klimek wrote: > > I'd have expected something that's called LSP server to work on the LSP > > protocol level

[PATCH] D33047: [ClangD] Refactor clangd into separate components

klimek added inline comments. Comment at: clangd/ClangdLSPServer.h:23 + +class ClangdLSPServer { + class LSPDiagnosticsConsumer; I'd have expected something that's called LSP server to work on the LSP protocol level (that is, have a server(iostream) equivalent

[PATCH] D32480: clang-format: Add CompactNamespaces option

klimek added a comment. In https://reviews.llvm.org/D32480#773807, @Typz wrote: > So how do I proceed? > > 1. Keep the CompactNamespace option, and make "compacted" namespaces always > add at most one level of indentation > 2. Or assume that this can only ever usefully work with the behavior of

[PATCH] D34329: [GSoC] Clang AST diffing

klimek added a comment. Reviewing this mainly from the API view, and leaving the technical details to others :) Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:124 + +class ASTDiff { + TreeRoot , Generally, can we put the public interface first in the

[PATCH] D34263: [preprocessor] When preprocessor option 'SingleFileParseMode' is enabled, parse all directive blocks if the condition uses undefined macros

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

[PATCH] D34263: [preprocessor] When preprocessor option 'SingleFileParseMode' is enabled, parse all directive blocks if the condition uses undefined macros

klimek added a comment. In https://reviews.llvm.org/D34263#784168, @akyrtzi wrote: > In https://reviews.llvm.org/D34263#783694, @klimek wrote: > > > how many patches for single file mode are coming down the road, though? I'm > > somewhat concerned about the overall complexity it'll add to

[PATCH] D34304: Allow CompilerInvocations to generate .d files.

klimek added a comment. In https://reviews.llvm.org/D34304#784496, @saugustine wrote: > In https://reviews.llvm.org/D34304#783675, @klimek wrote: > > > I think a better way might be to generally leave dependency options alone, > > add a default argument adapter to filter out all deps related

[PATCH] D34329: [GSoC] Clang AST diffing

klimek added inline comments. Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:123 + +void runDiff(ASTContext , ASTContext ); + This is the main exposed interface? Generally, if all we want to do is printing, I wouldn't put that into a library in Tooling,

[PATCH] D34287: Moved code hanlding precompiled preamble out of the ASTUnit.

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

[PATCH] D33823: [clang-format] Support sorting using declarations

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

[PATCH] D34287: Moved code hanlding precompiled preamble out of the ASTUnit.

klimek added inline comments. Comment at: lib/Frontend/ASTUnit.cpp:131-136 +/// \brief Get a source buffer for \p MainFilePath, handling all file-to-file +/// and file-to-buffer remappings inside \p Invocation. +static PossiblyOwnedBuffer +getBufferForFileHandlingRemapping(const

[PATCH] D34469: Use vfs::FileSystem in ASTUnit when creating CompilerInvocation.

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

[PATCH] D34470: [clangd] Allow to override resource dir in ClangdServer.

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

[PATCH] D34304: Allow CompilerInvocations to generate .d files.

klimek added a comment. I mean, arguments need to be adjusted before converting to ArgStringList and calling newInvocation? I'm not sure I fully understand the problem, can you elaborate? https://reviews.llvm.org/D34304 ___ cfe-commits mailing

[PATCH] D34329: [GSoC] Clang AST diffing

klimek added inline comments. Comment at: include/clang/Tooling/ASTDiff/ASTDiff.h:57 +/// Within a tree, this identifies a node by its preorder offset. +using NodeId = int; + johannes wrote: > arphaman wrote: > > I think that it's better to make make `NodeId` a

[PATCH] D30691: [analyzer] Support for naive cross translational unit analysis

klimek added a reviewer: rsmith. klimek added a comment. Richard (added as reviewer) usually owns decisions around clang itself. Writing an email to cfe-dev with the numbers and wait for whether others have concerns would probably also be good. https://reviews.llvm.org/D30691

[PATCH] D34287: Moved code hanlding precompiled preamble out of the ASTUnit.

klimek added inline comments. Comment at: include/clang/Frontend/PrecompiledPreamble.h:124 +/// CanReusePreamble + AddImplicitPreamble to make use of it. +class PrecompiledPreamble { +public: ilya-biryukov wrote: > klimek wrote: > > If a user doesn't care about

[PATCH] D34512: [libTooling] Add preliminary Cross Translation Unit support for libTooling

klimek added reviewers: dexonsmith, akyrtzi, benlangmuir, arphaman. klimek added a comment. Adding folks interested in the indexing discussion. Comment at: include/clang/Tooling/CrossTranslationUnit.h:53-58 + /// \p CrossTUDir directory, called \p IndexName. In case the

[PATCH] D34304: Allow CompilerInvocations to generate .d files.

klimek added a comment. In https://reviews.llvm.org/D34304#788080, @saugustine wrote: > In https://reviews.llvm.org/D34304#787699, @klimek wrote: > > > I mean, arguments need to be adjusted before converting to ArgStringList > > and calling newInvocation? I'm not sure I fully understand the

[PATCH] D34512: [libTooling] Add preliminary Cross Translation Unit support for libTooling

klimek added inline comments. Comment at: include/clang/Tooling/CrossTranslationUnit.h:53-58 + /// \p CrossTUDir directory, called \p IndexName. In case the declaration is + /// found in the index the corresponding AST file will be loaded and the + /// definition of the

[PATCH] D34512: [libTooling] Add preliminary Cross Translation Unit support for libTooling

klimek added a comment. General direction looks good. I think in addition to integration tests through the static analyzer, we should still have unit tests. Comment at: include/clang/Tooling/CrossTranslationUnit.h:47 + + const FunctionDecl *getCTUDefinition(const

[PATCH] D32480: clang-format: Add CompactNamespaces option

klimek added a comment. I'm less concerned about everything suddenly re-indenting when you change code - if you use any kind of namespace indentation, that's what will happen now and then (and is why many style guides do not indent in namespaces). For what it's worth, I'd 1. vote for only

[PATCH] D33714: clang-format: [JS] improve calculateBraceType heuristic

klimek accepted this revision. klimek added a comment. This revision is now accepted and ready to land. Apart from fixme LG. Comment at: lib/Format/UnwrappedLineParser.cpp:397 // blocks (for example while parsing lambdas). + // TODO(martinprobst): Some of

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

klimek added inline comments. Comment at: lib/Sema/SemaCodeComplete.cpp:2453 std::string PlaceholderStr = FormatFunctionParameter(Policy, Param); +if (Param->hasDefaultArg() && PlaceholderStr.find("=") == std::string::npos) { +std::string DefaultValue =

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

klimek added a comment. Ok, now I get it - can you please add tests? This is usually tested by adding a c-index-test based test. Comment at: lib/Sema/SemaCodeComplete.cpp:2411-2417 + const SourceLocation StartLoc = SrcRange.getBegin(); + const SourceLocation EndLoc =

[PATCH] D32480: clang-format: Add CompactNamespaces option

klimek added a comment. Generally LG from my side. Comment at: unittests/Format/FormatTest.cpp:1363-1381 + EXPECT_EQ("namespace {\n" + "namespace {\n" +"}} // namespace

[PATCH] D34304: Allow CompilerInvocations to generate .d files.

klimek added a comment. I think a better way might be to generally leave dependency options alone, add a default argument adapter to filter out all deps related flags, and allow users to add their own argument adapters that don't do that. https://reviews.llvm.org/D34304

[PATCH] D34263: [preprocessor] When preprocessor option 'SingleFileParseMode' is enabled, parse all directive blocks if the condition uses undefined macros

klimek added a comment. Generally this patch lg from my side - how many patches for single file mode are coming down the road, though? I'm somewhat concerned about the overall complexity it'll add to clang. When I saw your first patch my reaction was "wow, this works with this little change -

[PATCH] D33823: [clang-format] Support sorting using declarations

klimek added inline comments. Comment at: lib/Format/UsingDeclarationsSorter.cpp:41-42 + +bool computeUsingDeclarationLabel(const FormatToken *UsingTok, + std::string *Label) { + assert(UsingTok && UsingTok->is(tok::kw_using) && "Expecting a

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

klimek added inline comments. Comment at: lib/Sema/SemaCodeComplete.cpp:2453 std::string PlaceholderStr = FormatFunctionParameter(Policy, Param); +if (Param->hasDefaultArg() && PlaceholderStr.find("=") == std::string::npos) { +std::string DefaultValue =

[PATCH] D34287: Moved code hanlding precompiled preamble out of the ASTUnit.

klimek added inline comments. Comment at: include/clang/Frontend/PrecompiledPreamble.h:42-43 +/// destructors. +/// An assertion will fire if two PCHTempFiles are created with the same name, +/// so it's not intended to be used outside preamble-handling. +class TempPCHFile {

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

klimek added a comment. Can you give a bit more background what this is trying to do? https://reviews.llvm.org/D33644 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32351: [Tooling][libclang] Remove unused CompilationDatabase::MappedSources

klimek added a comment. Is there a specific reason to take this out? It seems generally useful to allow compilation-db implementors to provide sources. https://reviews.llvm.org/D32351 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D32909: [Tooling] Remove redundant check, NFCi

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

[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

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

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

<    1   2   3   4   5   6   >