klimek (Manuel Klimek)Administrator
User

Projects

User does not belong to any projects.

User Details

User Since
Jul 7 2012, 2:55 PM (276 w, 1 d)
Roles
Administrator

Recent Activity

Tue, Oct 17

klimek added a comment to D5767: Template Instantiation Observer + a few other templight-related changes.

Whee, thanks for driving this, much appreciated :D

Tue, Oct 17, 3:18 PM
klimek added inline comments to D34272: [Tooling] A new framework for executing clang frontend actions..
Tue, Oct 17, 9:10 AM

Thu, Oct 12

klimek added a comment to D38818: Template Instantiation Observer + a few other templight-related changes.

Please make sure patches get sent to cfe-commits in addition to the reviewers, as per https://llvm.org/docs/Phabricator.html

Thu, Oct 12, 2:16 PM
klimek added a comment to D37979: ClangFormat - Add one space in inline empty function that can be wrapped on a single line. .

I also see lots of code that uses {}? Thus, it seems like the code base is not very strict, and going either way enforced with clang-format will be nicer?
https://dxr.mozilla.org/mozilla-central/search?q=%22+%7B%7D&redirect=true

Indeed, this is why we are working on enforcing that.
This patch is one of the last change to match exactly our coding style. This is why Andi wrote it.

If you could accept it, it would be great. If you won't, please let us know to see what we can do on our side. Thanks!

Thu, Oct 12, 7:51 AM

Wed, Oct 11

klimek added a comment to D37979: ClangFormat - Add one space in inline empty function that can be wrapped on a single line. .

Also doing a grep in our online repository:
https://dxr.mozilla.org/mozilla-central/search?q=%22%7B+%7D&redirect=false

You can see that the incidence of this behaviour is huge. I know the grep produces also noise but still the occurrence is this scenario is huge.

Wed, Oct 11, 4:14 PM

Thu, Oct 5

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

LG

Thu, Oct 5, 8:11 AM
klimek added inline comments to D38583: [clangd] Added async API to run code completion..
Thu, Oct 5, 6:51 AM
klimek added inline comments to D38583: [clangd] Added async API to run code completion..
Thu, Oct 5, 6:32 AM

Thu, Sep 28

klimek added a comment to D38298: A logic to copy LLVM licences into docker images..

"make install" is not primarily a distribution mechanism, docker is (most folks upload their docker images to public registries without thinking much about this).

I'm not sure why "primarily a distribution mechanism matters? How is one suppose to distribute?

docker push to a public registry?

Not much different from pushing a .tar.gz resulting from the install to llvm.org (which is actually that how I package my internal distribution right now, luckily I don't distribute outside the company, so I haven't hit the license problem...).

Thu, Sep 28, 9:04 AM
klimek added a comment to D38298: A logic to copy LLVM licences into docker images..

"make install" is not primarily a distribution mechanism, docker is (most folks upload their docker images to public registries without thinking much about this).

I'm not sure why "primarily a distribution mechanism matters? How is one suppose to distribute?

Thu, Sep 28, 2:59 AM
klimek added a comment to D38298: A logic to copy LLVM licences into docker images..

Can you elaborate why this script is desirable/needed?

Sure, sorry for leaving out the context.

Including licences is technically required in any binary distribution of LLVM, including ones inside Docker images.
Copying the licenses could be made optional, but always putting the licenses into Docker images should not hurt either.

OK, but why is this logic applicable to clang/llvm built with the docker script and not for any build?

It seems to me that if this is deemed useful/necessary, that should be part of make install and controlled by a CMake flag (-DLLVM_INSTALL_LICENCES=ON) which could be enabled by default or not.

Docker is great for managing dependencies and providing build reproducibility: it is great to provide scripts that makes it easier to use to build clang/LLVM. However I'm worried about embedding any information in these scripts about build / release / packaging that wouldn't be Docker specific.

Thu, Sep 28, 12:42 AM

Wed, Sep 27

klimek added inline comments to D38298: A logic to copy LLVM licences into docker images..
Wed, Sep 27, 4:18 AM

Mon, Sep 25

klimek added a comment to D36827: Changed createTemporaryFile without FD to actually create a file..

Is this still pending review?

Mon, Sep 25, 7:15 AM
klimek added a comment to D37979: ClangFormat - Add one space in inline empty function that can be wrapped on a single line. .

Can you point out where that's spelled out in the style guide? I can't find a reference to it (searching for "{}" or "{ }" doesn't provide any hits).
I'm slightly opposed to this, given that so far I haven't heard a convincing argument why this makes any difference to readability.

Mon, Sep 25, 5:10 AM
klimek added a comment to D37732: Add missing NL (new line) at EOF (end of file).

For what it's worth, I think it'd also be fine to always add a newline at the end of a file in clang-format, or have an option. I think this is fine, too, though.

Mon, Sep 25, 5:03 AM · Restricted Project

Sep 22 2017

klimek added a comment to D33722: [clang-tidy] Add checker for undelegated copy of base classes.

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).

Sep 22 2017, 6:06 AM · Restricted Project
klimek accepted D38151: [clang] Fix isExternC matcher docs.

LG

Sep 22 2017, 4:31 AM

Sep 21 2017

klimek added a reviewer for D37554: [libclang] Allow crash recovery with LIBCLANG_NOTHREADS: akyrtzi.

Adding Argyrios, who might have insight on how this is used.
I think this had the wrong list of reviewers so far :(

Sep 21 2017, 4:59 AM
klimek added a comment to D35743: [clang-format] Adjust space around &/&& of structured bindings.

Apart from nits, LGTM, unless Daniel sees issues.

Sep 21 2017, 4:20 AM · Restricted Project

Sep 20 2017

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

LG

Sep 20 2017, 4:48 AM
klimek added inline comments to D38032: [clangd] Serialize onDiagnosticsReady callbacks for the same file..
Sep 20 2017, 3:41 AM
klimek committed rL313744: clang-format clang-format..
clang-format clang-format.
Sep 20 2017, 2:52 AM
klimek added a comment to D35743: [clang-format] Adjust space around &/&& of structured bindings.
  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
Sep 20 2017, 2:38 AM · Restricted Project
klimek committed rL313742: Fix clang-format's detection of structured bindings..
Fix clang-format's detection of structured bindings.
Sep 20 2017, 2:31 AM
klimek added a comment to D37813: clang-format: better handle namespace macros.

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.

Sep 20 2017, 2:28 AM
klimek added a comment to D33589: clang-format: consider not splitting tokens in optimization.
In 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 case.
But the code at the end of breakProtrudingToken() is actually very similar to what you propose.

I can refactor the code to have two functions reflowProtrudingToken() and getExcessPenalty(), though that will add some significant redundancy: the problem is that even if we don't actually reflow, we still need (I think?) to handle whitespace replacement and update the state (to set Column, Stack.back().LastSpace, and Stack[i].BreakBeforeParameter, and call updateNextToken() ...). And therefore getExcessPenalty() should probably not just compute the penalty, it should also update the state and handle whitespace replacement...

Sep 20 2017, 1:47 AM

Sep 19 2017

klimek added a comment to D35743: [clang-format] Adjust space around &/&& of structured bindings.

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 be largely independent of this patch, though, so LG from my side.

Sep 19 2017, 8:45 AM · Restricted Project
klimek added a comment to D33589: clang-format: consider not splitting tokens in optimization.

I find the current semantics of the functions a bit surprising, specifically:
... reflowProtrudingToken(..., bool Reflow)
is really confusing me :)

Sep 19 2017, 6:48 AM
klimek added a comment to D37980: [clang-format] Better parsing of lambda captures with initializer expressions..

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 point where we were able to generalize it, but we never did. Let me know if you find any problems with the aforementioned change.

Sep 19 2017, 3:02 AM
klimek committed rL313622: Fix formatting of lambda introducers with initializers..
Fix formatting of lambda introducers with initializers.
Sep 19 2017, 3:01 AM
klimek added a comment to D35743: [clang-format] Adjust space around &/&& of structured bindings.

So we actually didn't need to change the UnwrappedLineParser? I assume we still incorrectly assume the structured bindings are labmdas then?

Sep 19 2017, 2:34 AM · Restricted Project

Sep 18 2017

klimek added a comment to D35743: [clang-format] Adjust space around &/&& of structured bindings.
In 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 additional to kw_new, ke_delete, etc.

Could you suggest if there is any better way to handle the new syntax?

Sep 18 2017, 8:12 AM · Restricted Project
klimek added a reviewer for D37903: Fix assume-filename handling in clang-format.el: phst.

+Philipp, who is our emacs expert :)

Sep 18 2017, 2:13 AM

Sep 15 2017

klimek accepted D37099: Added optional validation of svn sources to Dockerfiles..

LG

Sep 15 2017, 4:59 AM

Sep 14 2017

klimek added inline comments to D37099: Added optional validation of svn sources to Dockerfiles..
Sep 14 2017, 5:35 AM

Sep 12 2017

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

LG. Nice, thanks!

Sep 12 2017, 12:49 AM

Sep 11 2017

klimek accepted D37634: clang-rename: let -force handle multiple renames.

LG

Sep 11 2017, 1:20 AM
klimek added inline comments to D37150: [clangd] Command line arg to specify compile_commands.json path.
Sep 11 2017, 1:05 AM
klimek added inline comments to D37663: [AST] Make RecursiveASTVisitor visit CXXOperatorCallExpr in source order.
Sep 11 2017, 12:59 AM
klimek accepted D37662: [AST] Make RecursiveASTVisitor visit TemplateDecls in source order.

LG

Sep 11 2017, 12:48 AM

Sep 5 2017

klimek added inline comments to D35943: [clang-format] Format raw string literals.
Sep 5 2017, 12:31 AM

Sep 4 2017

klimek added inline comments to D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action.
Sep 4 2017, 5:09 AM
klimek added a comment to D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action.

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?

@klimek, are you talking about the template usage in this patch or the whole approach in general? I can probably think of some way to reduce the template boilerplate if you are talking about the general approach.

Sep 4 2017, 3:29 AM

Sep 1 2017

klimek added a comment to D36574: [refactor] add clang-refactor tool with initial testing support and local-rename action.

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?

Sep 1 2017, 7:38 AM

Aug 31 2017

klimek added a comment to D37291: [refactor] Use a RefactoringResultConsumer instead of tagged refactoring rule classes.

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

Aug 31 2017, 7:48 AM
klimek added inline comments to D37099: Added optional validation of svn sources to Dockerfiles..
Aug 31 2017, 2:15 AM

Aug 30 2017

klimek added inline comments to D37099: Added optional validation of svn sources to Dockerfiles..
Aug 30 2017, 7:04 AM
klimek added a comment to D13599: Add a way to get the CXCursor that corresponds to a completion result..

Long time no see - sorry for the delay. I want to pick this up again. Would you say I should add the tests as separate tests (i.e. something like -code-completion-cursors-at), or should I rather output the cursor in addition when requesting -code-completion-at, and then extend all existing unit tests with the cursor information?

As it's a new function, I'd add a separate test.

OK, cool. But to get high coverage we'd probably need to duplicate a lot of of the existing tests. Anyhow, I'm find with either approach as long as we can move this forward. @svenbrauch, will you take care of this? It would probably be enough for now to get some basic testing in.

Aug 30 2017, 1:26 AM
klimek added a comment to D13599: Add a way to get the CXCursor that corresponds to a completion result..

Long time no see - sorry for the delay. I want to pick this up again. Would you say I should add the tests as separate tests (i.e. something like -code-completion-cursors-at), or should I rather output the cursor in addition when requesting -code-completion-at, and then extend all existing unit tests with the cursor information?

Aug 30 2017, 1:05 AM

Aug 28 2017

klimek added inline comments to D36075: [refactor] Initial support for refactoring action rules .
Aug 28 2017, 8:21 AM
klimek added inline comments to D36075: [refactor] Initial support for refactoring action rules .
Aug 28 2017, 8:02 AM
klimek accepted D37213: Changed Dockerfiles to install LLVM into /usr/local.

LG - / was definitely wrong :)

Aug 28 2017, 7:56 AM

Aug 11 2017

klimek added inline comments to D36397: [clangd] Fixed a data race..
Aug 11 2017, 3:20 AM
klimek added a comment to D35955: clang-format: Add preprocessor directive indentation.

LG from my side.

Aug 11 2017, 2:41 AM · Restricted Project
klimek accepted D36398: [clangd] Check if CompileCommand has changed on forceReparse..

lg

Aug 11 2017, 1:51 AM
klimek accepted D36261: [clangd] Use multiple working threads in clangd..

lg

Aug 11 2017, 1:46 AM

Aug 10 2017

klimek accepted D36529: Fixed a race condition in PrecompiledPreamble..

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 clear.

Aug 10 2017, 4:38 AM
klimek added inline comments to D36529: Fixed a race condition in PrecompiledPreamble..
Aug 10 2017, 3:18 AM

Aug 9 2017

klimek accepted D35355: Fix templated type alias completion when using global completion cache.

LG

Aug 9 2017, 3:02 AM
klimek added a comment to D35955: clang-format: Add preprocessor directive indentation.

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 index into the data structures at 0 :)

Should I do one of these things? Let me know how you'd like me to implement this.

Aug 9 2017, 1:47 AM · Restricted Project

Aug 8 2017

klimek added a comment to D35955: clang-format: Add preprocessor directive indentation.

Manuel: Can you take a look at the last comment here? Why does PPBranchLevel start at -1?

Aug 8 2017, 9:04 AM · Restricted Project
klimek added a comment to D36398: [clangd] Check if CompileCommand has changed on forceReparse..

Also missing tests :)

Aug 8 2017, 1:17 AM
klimek added a comment to D36261: [clangd] Use multiple working threads in clangd..

High-level question: Why can't we use llvm::ThreadPool?

Aug 8 2017, 1:11 AM
klimek accepted D36136: clang-format: [JS] fix union type spacing in object & array types..

lg

Aug 8 2017, 12:58 AM
klimek accepted D36159: clang-format: [JS] handle single lines comments ending in `\\`..

lg

Aug 8 2017, 12:56 AM

Aug 7 2017

klimek added a comment to D36397: [clangd] Fixed a data race..

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 couldn't come up with an example that would reproduce this.

Aug 7 2017, 8:48 AM
klimek added a comment to D36397: [clangd] Fixed a data race..

Tests?

Aug 7 2017, 8:22 AM
klimek added inline comments to D36398: [clangd] Check if CompileCommand has changed on forceReparse..
Aug 7 2017, 8:01 AM
klimek added inline comments to D35943: [clang-format] Format raw string literals.
Aug 7 2017, 6:43 AM
klimek added inline comments to D35943: [clang-format] Format raw string literals.
Aug 7 2017, 4:44 AM
klimek added inline comments to D35943: [clang-format] Format raw string literals.
Aug 7 2017, 3:09 AM
klimek added inline comments to D35943: [clang-format] Format raw string literals.
Aug 7 2017, 1:35 AM

Aug 4 2017

klimek added a comment to D35355: Fix templated type alias completion when using global completion cache.

Sorry for missing this - can you add a test?

Aug 4 2017, 6:46 AM
klimek accepted D36308: Add handling for DeducedType to HasDeclarationMatcher .

Thanks! LG after comment change.

Aug 4 2017, 6:45 AM
klimek added inline comments to D35943: [clang-format] Format raw string literals.
Aug 4 2017, 6:26 AM
klimek added inline comments to D35943: [clang-format] Format raw string literals.
Aug 4 2017, 2:25 AM
klimek committed rL310041: Fix typo and update documentation..
Fix typo and update documentation.
Aug 4 2017, 1:42 AM

Aug 3 2017

klimek added inline comments to D36261: [clangd] Use multiple working threads in clangd..
Aug 3 2017, 1:38 AM

Aug 2 2017

klimek committed rL309810: Adapt clang-tidy checks to changing semantics of hasDeclaration..
Adapt clang-tidy checks to changing semantics of hasDeclaration.
Aug 2 2017, 6:14 AM
klimek closed D36154: Adapt clang-tidy checks to changing semantics of hasDeclaration. by committing rL309810: Adapt clang-tidy checks to changing semantics of hasDeclaration..
Aug 2 2017, 6:14 AM
klimek added a comment to D36184: [clang-diff] Filter AST nodes.

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).

Aug 2 2017, 6:08 AM
klimek committed rL309809: Unify and simplify the behavior of the hasDeclaration matcher..
Unify and simplify the behavior of the hasDeclaration matcher.
Aug 2 2017, 6:05 AM
klimek closed D27104: Unify and simplify the behavior of the hasDeclaration matcher. by committing rL309809: Unify and simplify the behavior of the hasDeclaration matcher..
Aug 2 2017, 6:05 AM
klimek added a comment to D36184: [clang-diff] Filter AST nodes.

Why? Also, missing tests.

Aug 2 2017, 3:46 AM
klimek added inline comments to D36185: [clang-diff] Fix similarity computation.
Aug 2 2017, 3:45 AM
klimek added inline comments to D36186: [clang-diff] Improve and test getNodeValue.
Aug 2 2017, 3:43 AM
klimek added a reviewer for D36187: [clang-diff] Use the relative name for NamedDecls: bkramer.

+Ben who I believe had written a similar function once.

Aug 2 2017, 3:11 AM
klimek added inline comments to D36156: [rename] Introduce symbol occurrences.
Aug 2 2017, 3:00 AM
klimek added a comment to D36154: Adapt clang-tidy checks to changing semantics of hasDeclaration..

ptal

Aug 2 2017, 12:57 AM
klimek updated the diff for D36154: Adapt clang-tidy checks to changing semantics of hasDeclaration..

Address review comment.

Aug 2 2017, 12:31 AM

Aug 1 2017

klimek accepted D36155: Use VFS operations in FileManager::makeAbsolutePath..

Nice catch! lg

Aug 1 2017, 11:59 PM
klimek created D36154: Adapt clang-tidy checks to changing semantics of hasDeclaration..
Aug 1 2017, 8:32 AM
klimek added a comment to D35012: [refactor] Add the AST source selection component.

Apart from nits, looks OK to me - Eric, are all your concerns addressed?

Aug 1 2017, 3:15 AM

Jul 28 2017

klimek added a reviewer for D27104: Unify and simplify the behavior of the hasDeclaration matcher.: bkramer.

Adding Ben as reviewer.

Jul 28 2017, 2:26 AM
klimek updated the diff for D27104: Unify and simplify the behavior of the hasDeclaration matcher..

Update to handle hasDeclaration for type alias template decls.

Jul 28 2017, 2:22 AM

Jul 27 2017

klimek added inline comments to D35943: [clang-format] Format raw string literals.
Jul 27 2017, 8:30 AM

Jul 25 2017

klimek committed rL308969: Fix spelling of FileCheck in test..
Fix spelling of FileCheck in test.
Jul 25 2017, 4:35 AM
klimek committed rL308962: Fix incorrect use of current directory to find moved paths in ASTReader..
Fix incorrect use of current directory to find moved paths in ASTReader.
Jul 25 2017, 3:24 AM
klimek closed D35828: Fix incorrect use of current directory to find moved paths in ASTReader. by committing rL308962: Fix incorrect use of current directory to find moved paths in ASTReader..
Jul 25 2017, 3:24 AM
klimek created D35828: Fix incorrect use of current directory to find moved paths in ASTReader..
Jul 25 2017, 3:04 AM