klimek (Manuel Klimek)Administrator
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Fri, Aug 11

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

LG from my side.

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

lg

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

lg

Fri, Aug 11, 1:46 AM

Thu, Aug 10

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.

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

Wed, Aug 9

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

LG

Wed, Aug 9, 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.

Wed, Aug 9, 1:47 AM · Restricted Project

Tue, Aug 8

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?

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

Also missing tests :)

Tue, Aug 8, 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?

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

lg

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

lg

Tue, Aug 8, 12:56 AM

Mon, Aug 7

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.

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

Tests?

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

Fri, Aug 4

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?

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

Thanks! LG after comment change.

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

Thu, Aug 3

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

Wed, Aug 2

klimek committed rL309810: Adapt clang-tidy checks to changing semantics of hasDeclaration..
Adapt clang-tidy checks to changing semantics of hasDeclaration.
Wed, Aug 2, 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..
Wed, Aug 2, 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).

Wed, Aug 2, 6:08 AM
klimek committed rL309809: Unify and simplify the behavior of the hasDeclaration matcher..
Unify and simplify the behavior of the hasDeclaration matcher.
Wed, Aug 2, 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..
Wed, Aug 2, 6:05 AM
klimek added a comment to D36184: [clang-diff] Filter AST nodes.

Why? Also, missing tests.

Wed, Aug 2, 3:46 AM
klimek added inline comments to D36185: [clang-diff] Fix similarity computation.
Wed, Aug 2, 3:45 AM
klimek added inline comments to D36186: [clang-diff] Improve and test getNodeValue.
Wed, Aug 2, 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.

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

ptal

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

Address review comment.

Wed, Aug 2, 12:31 AM

Tue, Aug 1

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

Nice catch! lg

Tue, Aug 1, 11:59 PM
klimek created D36154: Adapt clang-tidy checks to changing semantics of hasDeclaration..
Tue, Aug 1, 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?

Tue, Aug 1, 3:15 AM

Fri, Jul 28

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

Adding Ben as reviewer.

Fri, Jul 28, 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.

Fri, Jul 28, 2:22 AM

Thu, Jul 27

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

Tue, Jul 25

klimek committed rL308969: Fix spelling of FileCheck in test..
Fix spelling of FileCheck in test.
Tue, Jul 25, 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.
Tue, Jul 25, 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..
Tue, Jul 25, 3:24 AM
klimek created D35828: Fix incorrect use of current directory to find moved paths in ASTReader..
Tue, Jul 25, 3:04 AM

Mon, Jul 24

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

lg

Mon, Jul 24, 7:43 AM
klimek added a comment to D35783: Ignore shadowing for declarations coming from within macros..

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.

Mon, Jul 24, 4:34 AM
klimek added a comment to D35783: Ignore shadowing for declarations coming from within macros..

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
Mon, Jul 24, 12:40 AM

Jul 18 2017

klimek added a comment to D34440: [Clang] Expand response files before loading compilation database.

If you want one unified format, the compilation database is it.

Is the clang-tidy ... -- <args> meant to be more or less a drop-in replacement for clang <args> (arguments-wise)?
If yes, this expansion of response files here is an another step in this direction.

Jul 18 2017, 7:26 AM · Restricted Project
klimek added inline comments to D35012: [refactor] Add the AST source selection component.
Jul 18 2017, 7:08 AM
klimek added inline comments to D35012: [refactor] Add the AST source selection component.
Jul 18 2017, 6:35 AM
klimek accepted D33644: Add default values for function parameter chunks.

lg

Jul 18 2017, 5:30 AM
klimek added a comment to D33644: Add default values for function parameter chunks.

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)

Jul 18 2017, 5:02 AM
klimek added a comment to D33309: Add autoload cookies for clang-include-fixer lisp functions..

Submitted as r308290.

Jul 18 2017, 3:15 AM
klimek committed rL308290: Add autoload cookies for clang-include-fixer lisp functions..
Add autoload cookies for clang-include-fixer lisp functions.
Jul 18 2017, 3:15 AM
klimek added inline comments to D35012: [refactor] Add the AST source selection component.
Jul 18 2017, 1:39 AM

Jul 17 2017

klimek committed rL308185: Fix dereference of pointers in throw statements..
Fix dereference of pointers in throw statements.
Jul 17 2017, 8:28 AM
klimek added inline comments to D35012: [refactor] Add the AST source selection component.
Jul 17 2017, 4:44 AM

Jul 14 2017

klimek added a comment to D34440: [Clang] Expand response files before loading compilation database.

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 automatically by CMake, while other build systems may not do it.
So we need to generate it on the fly (by tool or by hand), which also can lead to hidden problems due to different formats, different escaping rules, etc.
I think it's always good to have one unified format.

Jul 14 2017, 7:40 AM · Restricted Project
klimek added a comment to D34440: [Clang] Expand response files before loading compilation database.

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=* main.cpp -export-fixes=... -- @arguments.rsp
it also has no effect, because all compiler options will be ignored (i thinks it's because that stripPositionalArgs() function deletes @arguments.rsp parameter as unused input).

Jul 14 2017, 5:40 AM · Restricted Project
klimek added a comment to D35404: Email Test 2.

That worked?

Jul 14 2017, 1:31 AM
klimek added a comment to D34440: [Clang] Expand response files before loading compilation database.

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 widely used in Clang-Tidy and in other common tools.
Anyway, there are possibility to pass compiler options directly through command line.
But we cannot pass compiler options directly through command-line, due to command-line-length restrictions.
So, we decided to pass compiler options through @ResponseFile mechanism.
But there are some problems with this approach.
Before this patch, /clang/lib/Tooling/CommonOptionsParser.cpp worked in this way:

  1. Load compilation database from command-line
  2. Expand response files
  3. Parse all other command line arguments I think it's strange behavior? Why we try to load compilation database first? Maybe it's better to expand response files and only after that try to load compilation database and parse another options? It's hard to refactor cl::ParseCommandLineOptions and cl::ExpandResponseFiles functions, due to high number of usages, so i copied some block directly into /clang/lib/Tooling/CommonOptionsParser.cpp I don't think that is a best solution, but i don't have another one.
Jul 14 2017, 1:27 AM · Restricted Project
klimek accepted D34267: do more processing in clang-fuzzer (use EmitAssemblyAction).

lg

Jul 14 2017, 1:20 AM

Jul 12 2017

klimek accepted 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.

lg

Jul 12 2017, 7:19 AM
klimek added a reviewer for D33644: Add default values for function parameter chunks: rsmith.

+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?

Jul 12 2017, 7:06 AM
klimek added a comment to D34512: Add preliminary Cross Translation Unit support library.

Yep, I want Richard's approval for the name. I think he already expressed a pro-pulling-out stance.

Jul 12 2017, 7:01 AM

Jul 11 2017

klimek accepted D35211: Use new command replace-buffer-contents if available.

LG

Jul 11 2017, 1:52 AM

Jul 10 2017

klimek added inline comments to D33644: Add default values for function parameter chunks.
Jul 10 2017, 8:43 AM
klimek added a comment to D34512: Add preliminary Cross Translation Unit support library.

Specifically, ping Richard for new top-level lib in clang.

Jul 10 2017, 8:38 AM
klimek accepted D35197: Improve error message when run from a buffer that doesn't visit a file.

lg

Jul 10 2017, 5:24 AM

Jul 7 2017

klimek added a comment to D35066: Add "Skip files" option to clang-format plugin for VS.

are you making changes to the generated file, and want to not format those changes?

Yes. Sometimes we change IDs on all code including resource.h, but for this specific file
we want to keep the formatting set by Visual Studio resource editor.

Jul 7 2017, 5:48 AM · Restricted Project
klimek added a comment to D35066: Add "Skip files" option to clang-format plugin for VS.

Looking at the code, probably the dirty check should be done sooner than later:

if (!options.FormatOnSave || !Vsix.IsDocumentDirty(document))
    return;

Where are you seeing this? In ClangFormatPackge.cs it looks like we're returning if the document's not dirty?

This was just a proposed change for ClangFormatPackge.cs.
I think it makes sense to move this check above the file extensions check because the later is useless if the document is not dirty.

Of course, I just assume that IsDocumentDirty() call is cheaper than FileHasExtension() + FileIsIgnored() calls.

Let me know if you would like to upload a new diff than contains this small optimisation too.

Jul 7 2017, 5:28 AM · Restricted Project
klimek added a comment to D35066: Add "Skip files" option to clang-format plugin for VS.

No, this was not changed and works just fine. Format on save triggers only if there are changes.

Looking at the code, probably the dirty check should be done sooner than later:

if (!options.FormatOnSave || !Vsix.IsDocumentDirty(document))
    return;
Jul 7 2017, 3:20 AM · Restricted Project
klimek added a comment to D35066: Add "Skip files" option to clang-format plugin for VS.

Assuming you're not changing / saving a generated file, I'm wondering whether the real bug is that format-on-save triggers if nothing in the file changed?

Jul 7 2017, 1:21 AM · Restricted Project

Jul 6 2017

klimek added a comment to D34512: Add preliminary Cross Translation Unit support library.

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 now), I think we could start/continue the review of this patch.

I read that somewhat differently? It seems like Richard basically proposes adding a new library for things that control how we run clang in a multi-TU scenario. I'd call it libIndex, but that already exists :)

No, but since Richard said he did not see any justifications to include this in libTooling, I had the impression this is not his final word, and in case you see it justified, this remains the suggested direction.
But in this case I will rewrite this patch to create a new library. Are you still interested in reviewing it? Do you have any other name in mind? What about libCrossTU?

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

Some brainstorming for the bikeshedding: libProjet, libProjectIndex, libImport
The reason I am not sure about the Index in the name because this code does not contain (yet) an interface to create/handle indexes.
The import might be a good once, since this library is basically wrapping the ASTImporter to import code from external sources, but it might be misleading, since ASTImporter is in the AST module.

Cross-TU is now used (in the lifeline of these features introduced by the team) in multiple contexts: CTU analysis, and now CTU lookup.
Maybe the name of this lib should reflect on the fact that the lib is used to support cross-TU definition lookup.

Hmm... libCrossTULocator?

Jul 6 2017, 6:43 AM
klimek added a reviewer for D34512: Add preliminary Cross Translation Unit support library: rsmith.

+Richard as top-level code owner for new libs.

Jul 6 2017, 5:55 AM
klimek accepted D35050: Made a script to build docker images easier to use..

lg

Jul 6 2017, 5:25 AM
klimek added a comment to D34512: Add preliminary Cross Translation Unit support library.

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 now), I think we could start/continue the review of this patch.

Jul 6 2017, 3:08 AM

Jul 5 2017

klimek accepted D33309: Add autoload cookies for clang-include-fixer lisp functions..

LG

Jul 5 2017, 7:30 AM
klimek added a comment to D34197: Added Dockerfiles to build clang from sources..

Thanks! LGTM.

I'll be happy to iterate separately on providing an easy way to build a boostrap of clang (that would be easy to use with or without Docker)!

As a data point - I just tried to build the docker image and without Ilya's help I wouldn't have known to look at the docs (usually docker build envs for projects just work out of the box to give me a full image; I don't know whether you have a different experience). We'll improve comments on the script itself, but as a user I'd really like to have something that I can run that will Just Work :)

I don't disagree that having something that "just works" would be nice. However you have to take into account that LLVM is a very complex project and there is not a "single image" that can fit all.

If we start with an "easy to build" full image, it should be the same as the official packages: http://releases.llvm.org ; this is *not* what was proposed here.

Jul 5 2017, 12:32 AM

Jul 4 2017

klimek added a comment to D34197: Added Dockerfiles to build clang from sources..

Thanks! LGTM.

I'll be happy to iterate separately on providing an easy way to build a boostrap of clang (that would be easy to use with or without Docker)!

Jul 4 2017, 1:47 AM

Jun 30 2017

klimek accepted D34696: [refactor] Move clang-rename to Clang.

lg

Jun 30 2017, 7:22 AM
klimek accepted D34304: Allow CompilerInvocations to generate .d files..

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

Jun 30 2017, 3:20 AM

Jun 29 2017

klimek added a comment to D34696: [refactor] Move clang-rename to Clang.

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?

Would it be better if I make a patch for the clang-refactor tool first and then update this patch to support some of clang-rename's functionality in clang-refactor and move the tests over? Or should I just move all of clang-rename and its tests to clang's repository first and then focus on clang-refactor?

Jun 29 2017, 3:25 AM

Jun 28 2017

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

lg

Jun 28 2017, 8:04 AM
klimek added a comment to D34696: [refactor] Move clang-rename to Clang.

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?

Jun 28 2017, 5:13 AM
klimek added a comment to D33644: Add default values for function parameter chunks.

Do not evaluate numbers.
Check for != "=" is needed not to mess with invalid default arguments or their types (without it I get "const Bar& bar = =" when Bar is not defined)

Shouldn't we than instead check that error case?

I don't know the proper way to do that :) I also don't know the full amount of errors that might cause such behavior.
You can suggest the solution if you have some idea. Current one is safe because we just ignore any case that causes empty default string.

Taking your example "const Bar& bar = =" when Bar is not defined:
What happens now? Will it be "const Bar& bar ="? I argue that is not better than "const Bar& bar = =".
Btw, can you add tests? I think that will make it easier to see what's actually happening.

It will be just "const Bar& bar" in that case with this patch applied. So it is better than both "const Bar& bar =" and "const Bar& bar = =" :)
About tests - I agree that it's nice to have them. Can I find something similar in the current test set (as an example) and where is it better to put my tests?

Jun 28 2017, 4:43 AM
klimek added a comment to D33644: Add default values for function parameter chunks.

Do not evaluate numbers.
Check for != "=" is needed not to mess with invalid default arguments or their types (without it I get "const Bar& bar = =" when Bar is not defined)

Shouldn't we than instead check that error case?

I don't know the proper way to do that :) I also don't know the full amount of errors that might cause such behavior.
You can suggest the solution if you have some idea. Current one is safe because we just ignore any case that causes empty default string.

Jun 28 2017, 4:28 AM
klimek added a comment to D33644: Add default values for function parameter chunks.

Do not evaluate numbers.
Check for != "=" is needed not to mess with invalid default arguments or their types (without it I get "const Bar& bar = =" when Bar is not defined)

Jun 28 2017, 3:38 AM
klimek accepted D34687: [Tooling] CompilationDatabase should be able to strip position arguments when `-fsyntax-only` is used.

lg

Jun 28 2017, 3:25 AM

Jun 26 2017

klimek added a reviewer for D30691: [analyzer] Support for naive cross translational unit analysis: rsmith.

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.

Jun 26 2017, 7:14 AM

Jun 23 2017

klimek added inline comments to D34512: Add preliminary Cross Translation Unit support library.
Jun 23 2017, 4:38 AM
klimek added reviewers for D34512: Add preliminary Cross Translation Unit support library: dexonsmith, akyrtzi, benlangmuir, arphaman.

Adding folks interested in the indexing discussion.

Jun 23 2017, 3:26 AM
klimek added a comment to D34304: Allow CompilerInvocations to generate .d files..

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?

This gets back to why the original patch plumbed the boolean all the way down to newInvocation.

newInvocation is the function that actually discards the dependency file options--today unconditionally.

Jun 23 2017, 1:51 AM

Jun 22 2017

klimek added a comment to D34512: Add preliminary Cross Translation Unit support library.

General direction looks good.

Jun 22 2017, 7:25 AM
klimek accepted D34453: [clang-format] Add a SortUsingDeclaration option and enable it by default..

LG. When you submit, please note the other fixes in the commit message (or make it 2 commits)

Jun 22 2017, 1:53 AM
klimek added inline comments to D34329: [clang-diff] Initial implementation..
Jun 22 2017, 1:31 AM