Page MenuHomePhabricator

klimek (Manuel Klimek)Administrator
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Today

klimek accepted D58404: [clang-format] Add basic support for formatting C# files.

LG. This looks pretty sharp (scnr).

Thu, Mar 21, 3:26 AM · Restricted Project, Restricted Project
klimek added a comment to D42034: [clang-format] In tests, expected code should be format-stable.

Great idea! LG from my side after addressing MyDeveloperDay's comments.

Thu, Mar 21, 2:08 AM · Restricted Project

Yesterday

klimek added inline comments to D55170: [clang-format]: Add NonEmptyParentheses spacing option.
Wed, Mar 20, 7:09 AM · Restricted Project, Restricted Project
klimek added a comment to D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range.

I don't understand yet why running clang-format locally would not do the same change?

Wed, Mar 20, 6:11 AM · Restricted Project
klimek accepted D59546: [clang-format] structured binding in range for detected as Objective C.

lg

Wed, Mar 20, 6:10 AM · Restricted Project, Restricted Project
klimek added inline comments to D57687: [clang-format] Add style option AllowShortLambdasOnASingleLine.
Wed, Mar 20, 6:05 AM · Restricted Project
klimek added inline comments to D59408: [clang-format] [PR25010] Extend AllowShortIfStatementsOnASingleLine not working if an "else" statement is present.
Wed, Mar 20, 4:02 AM · Restricted Project
klimek added inline comments to D58404: [clang-format] Add basic support for formatting C# files.
Wed, Mar 20, 4:02 AM · Restricted Project, Restricted Project
klimek added inline comments to D28462: clang-format: Add new style option AlignConsecutiveMacros.
Wed, Mar 20, 3:51 AM · Restricted Project
klimek added inline comments to D55170: [clang-format]: Add NonEmptyParentheses spacing option.
Wed, Mar 20, 3:15 AM · Restricted Project, Restricted Project
klimek added a comment to D55170: [clang-format]: Add NonEmptyParentheses spacing option.

I don't think risk is what matters - in the end it's about cost, and cost is a very technical reason

I'm really sorry I'm not trying to be difficult its just over the years I keep seeing this being said in reviews? but what is the cost? I assume you don't mean a financial cost? could you define cost as a problem so we can address it? what would it take to make cost not an issue?

Wed, Mar 20, 3:15 AM · Restricted Project, Restricted Project
klimek added a comment to D55170: [clang-format]: Add NonEmptyParentheses spacing option.

@klimek I agree that the rule is somewhat arbitrary. However, it's the style rule of an established codebase with many users (I don't have a concrete number, but the project has 1400 stars on github). I've found this patch useful when contributing to JUCE and I thought others might too.

Wed, Mar 20, 3:11 AM · Restricted Project, Restricted Project

Mon, Mar 18

klimek added a comment to D55170: [clang-format]: Add NonEmptyParentheses spacing option.

I'm sorry for not having a positive answer here, but I'm not in favor of this change. The style rule looks like it introduces arbitrary complexity at a point where I don't understand at all how it matters. We cannot possibly support all style guides that come up with random variance. I do not think this option pulls its weight.

So is this a personal opinion or based on a technical reason (other than the normal mantra about risk & cost),

Mon, Mar 18, 1:43 AM · Restricted Project, Restricted Project

Fri, Mar 15

klimek added a comment to D40988: Clang-format: add finer-grained options for putting all arguments on one line.

So @russellmcc you've been bumping along this road nicely for 6 months, doing what people say... pinging every week or so in order to get your code reviewed, and you are getting no response.

Was there anything you think people were objecting too other than the normal "its a high bar to get in" and "its complex in here"?

I think its fairer to the person submitting a revision if they don't want feature in, we should give feedback as to why, but are we to assume silence is acceptance? how long do we wait? who is the decision maker?

I've personally never met an Engineer who didn't like having more knobs to fiddle with so I don't believe the rational that having more options is bad as long as they don't interfere with each other, for that there is the "Beyonce rule", if adding an option breaks someone else then "if they liked it they should have put a test on it!"

As far as I can tell this LGTM (I'm not the code owner, just someone wanting to help)

In the meantime I have uploaded this patch to my fork, where I'm maintaining a clang-format with revisions that seem ok, but get stalled in the review process.

https://github.com/mydeveloperday/clang-experimental

Fri, Mar 15, 2:12 AM

Thu, Mar 14

klimek added a comment to D59309: [clang-format] BreakAfterReturnType ignored on functions with numeric template parameters.

Yea, the
a b(c)
problem is the olden C++ problem clang-format will never be able to solve, thus all these crazy heuristics.

Thu, Mar 14, 6:30 AM · Restricted Project
klimek added a comment to D59309: [clang-format] BreakAfterReturnType ignored on functions with numeric template parameters.

That works because the argument list is just tok::identifier and TT_StartOfName

Thu, Mar 14, 4:33 AM · Restricted Project
klimek added a comment to D38636: [clang-format] Cannot disable array initializer bin packing.

IIRC this was done due to performance problems in large nested initializer lists. Can you verify this doesn't regress performance?

Thu, Mar 14, 4:03 AM · Restricted Project, Restricted Project
klimek accepted D52150: [clang-format] BeforeHash added to IndentPPDirectives.

Given this doesn't add an extra option (but only an extra enum) and is fairly straight forward, LG.

Thu, Mar 14, 4:01 AM · Restricted Project
klimek added a comment to D28462: clang-format: Add new style option AlignConsecutiveMacros.

In regards to whether clang-format should support this feature, I think it's pretty clear that a large number of users want it to. The tool already supports formatting various other definitions in a similar manner, including variables and struct members. So I don't see how a similar features which aligns consecutive macros is something which doesn't make sense.

Additionally, because the formatting will *undo* such formatting if done manually, it means that the existing source code formatting this way cannot be handled via clang-format. In my case, it essentially means that I cannot use clang-format to enforce the style guidelines, as the macros definitions are aligned.

Additionally, aligning a series of macros makes sense because it helps improve readability, and is thus something that I'd rather not lose if I were to switch to using clang-format without the feature.

Thu, Mar 14, 3:55 AM · Restricted Project
klimek added a comment to D31635: [clang-format] Added ReferenceAlignmentStyle option.

Can you provide some evidence:

  • that this is in a style guide that's used widely enough
  • why you believe this matters
Thu, Mar 14, 3:37 AM · Restricted Project, Restricted Project
klimek added a comment to D55170: [clang-format]: Add NonEmptyParentheses spacing option.

Thanks for the review, and for approving this PR. It's very much appreciated!

I don't have merge rights - could someone commit this for me please?

I'd actively encourage you to go get commit access, I think its much better when the commit comes from the author.

There just isn't enough people actively involved in clang-format (and other tools) for us to get reviews even looked at (even for defects), we need more people involved fixing defects and doing reviews, getting commit access shows an intent to fix anything in that comes from your own review which is one of the things the code owners push as being an objection to adding more code.

Thu, Mar 14, 3:37 AM · Restricted Project, Restricted Project
klimek added a comment to D59309: [clang-format] BreakAfterReturnType ignored on functions with numeric template parameters.

Why did the numeric constant break this? Which path would the function run through?

as its looking though the parameter list is doesn't recognize a numeric constant as being a valid thing it would see in an argument list and so falls through the bottom to return false, it will only be where there is a numeric_constant as the first argument, if it has a second argument and that argument is just a simple type it would still fail, but switch them around and it won't

As I look more at this, even this would (and still would) fail to break

int TestFn(A=1)
{
  return a;
}

originally I didn't have the counting of the <> but one of the unit tests failed where we had

verifyFormat("LoooooooooooooooooooooooooooooooooooooooongType\n"
             "    LoooooooooooooooooooooooooooooooooooooooongVariable(1);");

I guess they wanted to do something like this when the type and name is very long

int
variable(1);

but I think this gets thought of as being a potential FunctionDecl

so I held off just adding tok::numeric_constant to the isOneOf(), and went with the use case

I think perhaps I could solve the default argument by adding a

startsSequence(tok::equal, tok::numeric_constant)

Thu, Mar 14, 3:27 AM · Restricted Project
klimek added a comment to D33029: [clang-format] add option for dangling parenthesis.

Adding the unit tests lets us see how this option will work in various cases, it will let us understand that its not breaking anything else.

I personally don't like to see revisions like this sit for 2 years with nothing happening, I don't see anything wrong with this that would prevent it going in so I don't understand whats blocking it?,

if you had some tests and a release note I'd give it a LGTM (but as I've said before I'm not the code owner, but someone wanting to address defects and add capabilities. but I think we need to be able to move forward, people will object soon enough if they don't like it.)

Generally I don't understand why clang-format is so reluctant to change anything, As such we don't have many people involved and getting anything done (even defects) is extremely hard.

It looks like you met the criteria, and reviewers have been given ample opportunity to make an objection. the number of subscribers and like tokens would suggest this is wanted,

Please also add a line the in the release notes to say what you are adding. In the absence of any other constructive input all we can do is follow the guidance on doing a review, for what its worth I notice in the rest of LLVM there seems to be a much larger amount of commits that go in even without a review, I'm not sure what makes this area so strict, so reluctant to change especailly when revisions do seem to be reviewed.

I don't have any stake here, but i just want to point out that no tool (including clang-format)
will ever support all the things all the people out there will want it to support. But every
new knob is not just a single knob, it needs to work well with all the other existing knobs
(and all of the combination of knob params), and every new knob after that.

It's a snowball effect. Things can (and likely will, unless there is at least a *very* strict testing/quality policy
(which is https://clang.llvm.org/docs/ClangFormatStyleOptions.html#adding-additional-style-options about))
get out of hand real quickly..

Just 2c.

Correct we should always be adding tests and show we don't break existing tests... We need to apply the "Beyonce Rule".. "if you liked it you should have put a test on it."

We shouldn't just give up making improvements or fixing defects because something is hard or complex.

Thu, Mar 14, 3:21 AM · Restricted Project
klimek added a comment to D59309: [clang-format] BreakAfterReturnType ignored on functions with numeric template parameters.

Why did the numeric constant break this? Which path would the function run through?

Thu, Mar 14, 2:49 AM · Restricted Project

Fri, Mar 1

klimek added a reviewer for D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling: gribozavr.
Fri, Mar 1, 3:03 AM · Restricted Project, Restricted Project, Restricted Project

Fri, Feb 22

klimek added a comment to D58345: [clangd] Using symbol name to map includes for STL symbols..

Sorry to be a pain here, but I'm not sure introducing Javascript is a good idea unless there's a strong reason for it.
All LLVM developers have python installed, many are comfortable with the language - it seems less likely to be a maintenance hazard. I can recommend BeautifulSoup for the HTML parsing stuff.
(For the record, I kind of hate python, but I'm more comfortable reviewing and maintaining it than JS).
Mitigating points: we have some in the VSCode plugin by necessity, and this isn't a build-time dependency.

Happy to get another opinion here.

Added the tool in this patch, but I think a separate repository under GitHub's clangd org might be a more suitable place.

I'm somewhat sympathetic to this idea, but I don't think we can do it as things stand. @klimek can confirm.

Fri, Feb 22, 6:27 AM · Restricted Project, Restricted Project

Thu, Feb 21

klimek added a comment to D28462: clang-format: Add new style option AlignConsecutiveMacros.

I'm not a code owner here but we seem to be lacking people who are either available, able, willing or allowed to approve code reviews for some of the code in the clang-tooling (not just clang format). The code owners do an extremely good job at doing a deep review way beyond what many of the rest of us are able to do.. They've been doing this for years having written may of these tools in the first place, but I suspect other external pressures and their phabricator activity suggests they have limited time to contribute at present.

As such code reviews for new features seem to sit unreviewed and unaccepted and yet the subscribers list and tokens, the pings and "whats blocking this" comments would suggest that revisions such as these are wanted but sit for weeks and months with no movement.

It feels like we need more people reviewing code and approving code changes otherwise these revisions bit rot or simply do not get into the tool. It definitely will disenfranchise new developers from contributing, if it takes months/years to get anything committed. But a shallow review by someone with at least a little experience of this code is surely better than no review at all. As such I'd like to volunteer to help review some of these items.

I don't think shallow reviews before having the architectural questions tied down would help a lot, as they in my experience create more opposition of change later on.

I fully agree that the state we're in is bad, and I'm truly sorry for us to have arrived here.

Not all areas of clang have this same problem, I see areas of code which sometimes don't even get a review, where buddies review each others code very quickly, or where the time from creation to commit can be measured in hours or days, not weeks or months. I'm unclear what makes commits here take so long and good improvements lost. As such I don't think we get people contributing to fixing bugs either because the time taken to get even a bug landed seems quite great.

One of the differences is whether there is ahead-of-code-review-time agreement on the direction, and whether the code fits in well from an architectural point of view.
In clang-format we specifically do not want to support all possible features, but carefully weigh the upside of features against maintenance cost. Given that formatting is a prototypical bikeshed situation, I know that this is often subjective and thus really frustrating, and again, I'm truly sorry for it, but I don't have a good solution :(

From what I understand as long as changes meet the guidelines set out about adding new options, changes have tests and have proper documentation then we should be free to approve and land those changes, The code owners have been given time like the rest of us to review and in the event they don't like the commit after the fact are free to submit a revision later or ask for further modifications. (make sure you check the done message on each of their review comments as you address them and ensure you leave a comment if you are not going to do the suggested change and why)

I think there's consensus between folks responsible for clang-format that we do specifically not want all features. The question is not only whether the code follows guidelines and is documented and tested. All code incurs cost and that has to be weighted carefully against the benefits.

As developers if we submit a revision we need to be prepared to look after that change and address and defects that come in, we don't want people driving by and dropping a patch that the code owners have to maintain, so with committing come responsibility.

I've personally been running with this patch in my clang format since @VelocityRa took it over, and from what I can tell it works well, I too would like to see this landed.

With that said @VelocityRa and this comment, I would ask that you add something to the docs/ReleaseNotes.rst to show the addition of the new option to clang-format, and if you make that change, rebase (I doubt anything else was accepted in the meantime!) and send an updated revision I will add a LGTM and accept the review (for what that is worth), this will give @djasper and @klimek time to object to my proposal here.

Assuming others are in agreement, lets do something to move at least this forward.

Thank you for responsing.. It seems no one is is able to approve code here without your or @djasper's input, of course we understand that is the ideal situation because you wrote this in the first place, and your ability to understand the code is far superior than others. But i'm sure you time is precious, how can the rest of us grow to help out?

I've heard the mantra before about "the cost of development", I'm not sure I understand why the "unit tests" aren't enough to prevent new styles breaking old capability, it feels like we don't have confidence that adding more won't break what is there already, and that is a lack of confidence that normally comes when code doesn't have enough tests, so why is clang-format so special that we can't cope with future additions? when the rest of clang (& C++ for that matter) is changing so rapidly underneath?

Thu, Feb 21, 8:28 AM · Restricted Project
klimek added a comment to D28462: clang-format: Add new style option AlignConsecutiveMacros.

I'm not a code owner here but we seem to be lacking people who are either available, able, willing or allowed to approve code reviews for some of the code in the clang-tooling (not just clang format). The code owners do an extremely good job at doing a deep review way beyond what many of the rest of us are able to do.. They've been doing this for years having written may of these tools in the first place, but I suspect other external pressures and their phabricator activity suggests they have limited time to contribute at present.

As such code reviews for new features seem to sit unreviewed and unaccepted and yet the subscribers list and tokens, the pings and "whats blocking this" comments would suggest that revisions such as these are wanted but sit for weeks and months with no movement.

It feels like we need more people reviewing code and approving code changes otherwise these revisions bit rot or simply do not get into the tool. It definitely will disenfranchise new developers from contributing, if it takes months/years to get anything committed. But a shallow review by someone with at least a little experience of this code is surely better than no review at all. As such I'd like to volunteer to help review some of these items.

Thu, Feb 21, 6:33 AM · Restricted Project

Jan 17 2019

klimek added a comment to D56841: [clangd] Filter out plugin related flags and move all commandline manipulations into OverlayCDB..

It seems like the most relevant place in tooling is ArgumentsAdjuster as @ioeric pointed out. Happy to move it to there if @sammccall and @klimek agrees as well.

As a side note ArgumentsAdjusters unfortunately causes a copy of the original command line arguments. Not sure how important this factor is compared to spinning up a compiler instance, just wanted to point it out.

Jan 17 2019, 6:38 AM

Jan 10 2019

klimek added a comment to D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on..

Just in general, I'd like to add that my experience over the years dealing with folks trying to do AST matchers is that the inability to express something is much more of a problem than matching too much, simply because the test cases people think of are usually small, and when running a transformation on a very large codebase, the cost of false negatives is significantly higher than the cost of false positive: the cost of a false negative means that you may produce something incorrect. The cost of a false positive is that you adapt your matcher to exclude the false negative.

Jan 10 2019, 4:16 AM

Jan 9 2019

klimek added a comment to D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on..

@klimek: would it be better to preserve the odd behavior of the functionDecl() matcher, and add a new functionOrLambdaDecl()? It seems too surprising to me.

We can change the clang-tidy check as well. I think lambdas should be considered functionDecls, shouldnt they? WDYT @aaron.ballman ?

I think it depends on whether we want the AST matchers to match what the user wrote (syntax) or how the program behaves (semantics). If the user writes a lambda, they did not syntactically write a function declaration and so it makes sense for functionDecl() to not match. However, the semantics of a lambda are that they act as a functor (in effect) which includes a class declaration, a function declaration for the function call operator, conversion operators, etc and so it does make sense for users to want to match those semantic components. Given that, I kind of think we should have functionDecl() match only functions, and give users some other way to match the semantic declarations in a consistent manner. Alternatively, we could decide semantics are what we want to match (because it's what the AST encodes) and instead we give users a way to request to only match syntax.

Jan 9 2019, 6:39 AM
klimek added a comment to D56444: [AST] RecursiveASTVisitor visits lambda classes when implicit visitation is on..

I still see the unit-test crashing for ExprMutAnalyzer (just apply the last two tests https://github.com/JonasToth/clang/blob/fix_crash/unittests/Analysis/ExprMutationAnalyzerTest.cpp and run check-clang-unit)

The ReproduceFailure11 test passes for me, and the ReproduceFailureMinimal fails to compile (a&&).

The clang-tidy check does not crash anymore, but readability-function-size regresses. It probably doesn't properly count for lambdas anymore.

Rather, it previously didn't count lambda bodies as functions (because functionDecl() didn't include them), and now it does :-)
I think counting them is better behavior, but I've modified the matcher to preserve the behavior for now.

@klimek: would it be better to preserve the odd behavior of the functionDecl() matcher, and add a new functionOrLambdaDecl()? It seems too surprising to me.

Jan 9 2019, 2:59 AM

Jan 4 2019

klimek added inline comments to D56090: Add a matcher for members of an initializer list expression.
Jan 4 2019, 2:14 AM · Restricted Project

Dec 11 2018

klimek added a comment to D54309: [AST] Allow limiting the scope of common AST traversals (getParents, RAV)..

See PR39949 as well, as it seems to trigger the same or a similar problem.
@ioeric and @klimek maybe could give an opinion, too?

Dec 11 2018, 5:55 AM

Dec 7 2018

klimek accepted D54672: clang-include-fixer.el: support remote files.

LG

Dec 7 2018, 6:26 AM

Nov 29 2018

klimek added inline comments to D54952: [clangd] DO NOT SUBMIT. Draft interfaces for build system integration..
Nov 29 2018, 6:32 AM

Nov 28 2018

klimek added inline comments to D54952: [clangd] DO NOT SUBMIT. Draft interfaces for build system integration..
Nov 28 2018, 2:28 AM

Nov 13 2018

klimek added a comment to D54453: Remove myself as owner of clang-query..

Thanks Aaron for volunteering, I'm very thankful for all your work on the reviews, it's much appreciated :D

Nov 13 2018, 1:21 AM

Nov 6 2018

klimek added a comment to D54077: [clangd] Implemented DraftFileSystem.

Someone mentioned to me that the interaction-between-features argument wasn't clear here:

  • we don't currently update diagnostics for A.cc when A.h is edited
  • we should, this seems more obvious & important than what we do with drafts
  • this interacts badly with using draft state, as this patch proposes - there are too many edits

FWIW, one of my "pain points" when using vim+clangd is:

  • Edit A.h in a buffer (and forget to save)
  • Switch to A.cc in another buffer
  • Realize that I forgot to save A.h
  • Go back to save A.h
  • Jump back to A.cc file.

    I would be very happy if A.cc can see the unsaved A.h when I am editing A.cc. I think the update should be relatively less frequent with this approach, as people don't usually update multiple files at the same time. Not sure if I want all files that depend on A.h to be updated when I edit A.h though; it seems much more complicated and less important (to me).
Nov 6 2018, 1:52 AM

Nov 5 2018

klimek added a comment to D54077: [clangd] Implemented DraftFileSystem.

@klimek If behavior will be configurable, is it ok for you?

Nov 5 2018, 7:05 AM
klimek added a comment to D54077: [clangd] Implemented DraftFileSystem.

Thanks for the patch! I believe many people I talked to want this behavior (myself included).
Some people like what we do now more. It feels like it depends on the workflow: for people who auto-save *all* files before build (some editors do it automatically?) the new behavior is the right one, for people who only save current file and rerun build on the console the old behavior is the correct one.
It all boils down to the argument "I want to see same errors that running the compiler would produce".

@klimek, @arphaman, @simark, WDYT?

Nov 5 2018, 4:15 AM

Oct 4 2018

klimek added a comment to D37813: clang-format: better handle namespace macros.

The problem here is that we have different opinions on how the formatting on namespace macros should behave in the first place- I think they should behave like namespaces, you want them to be formatted differently.
Given that you want full control over the formatting of those macros, and them not actually be formatted exactly like namespaces or classes, I think we need a more generic mechanism for you to express that.

Not sure what you mean here. I want them to behave like namespaces as well, this is actually the use case I have... As implemented, they indeed behave exactly like namespaces :

TESTSUITE(a) {                       namespace a {
} // TESTSUITE(a)                    } // namespace a
                                VS
TESTSUITE(a) { TESTSUITE(b) {        namespace a { namespace b {
} // TESTSUITE(a::b)                 }} // namespace a::b
Oct 4 2018, 2:35 AM

Sep 3 2018

klimek added inline comments to D44609: [Clang-Format] New option BeforeLambdaBody to manage lambda line break inside function parameter call (in Allman style).
Sep 3 2018, 1:04 AM · Restricted Project

Aug 27 2018

klimek added inline comments to D51258: Extract parseBindID method.
Aug 27 2018, 1:10 AM
klimek accepted D51259: Allow binding to NamedValue resulting from let expression.

LG

Aug 27 2018, 1:10 AM
klimek accepted D51261: Add preload option to clang-query.

LG

Aug 27 2018, 1:04 AM
klimek added inline comments to D44609: [Clang-Format] New option BeforeLambdaBody to manage lambda line break inside function parameter call (in Allman style).
Aug 27 2018, 12:45 AM · Restricted Project

Aug 21 2018

klimek accepted D50697: [clang-format] fix PR38557 - comments between "default" and ':' causes the case label to be treated as an identifier.

lg

Aug 21 2018, 3:40 AM

Aug 1 2018

klimek added a comment to D45719: [clang-Format] Fix indentation of member call after block.

Sorry that I lost track of that, but feel free to ping once / week - unfortunately the patch doesn't apply cleanly, can you rebase?

Aug 1 2018, 8:43 AM
klimek added a comment to D37813: clang-format: better handle namespace macros.

The problem here is that we have different opinions on how the formatting on namespace macros should behave in the first place- I think they should behave like namespaces, you want them to be formatted differently.
Given that you want full control over the formatting of those macros, and them not actually be formatted exactly like namespaces or classes, I think we need a more generic mechanism for you to express that.

Aug 1 2018, 3:15 AM
klimek added a comment to D50078: clang-format: support aligned nested conditionals formatting.

Yea, if we go down this route I'd go with this by default:

some ? thing :
else ? otherthing :
unrelated ? but :
    finally;
Aug 1 2018, 3:03 AM

Jul 26 2018

klimek added a comment to D49840: [AST] Add MatchFinder::matchSubtree.

Usually we use match(anyOf(node), hasDescendant(node)). Or did I misunderstand what you want?

Jul 26 2018, 3:21 AM · Restricted Project

Jul 24 2018

klimek accepted D49724: [VFS] Cleanups to VFS interfaces..

lg

Jul 24 2018, 4:45 AM

Jul 13 2018

klimek added inline comments to D44609: [Clang-Format] New option BeforeLambdaBody to manage lambda line break inside function parameter call (in Allman style).
Jul 13 2018, 2:28 AM · Restricted Project

Jul 3 2018

klimek added inline comments to D44609: [Clang-Format] New option BeforeLambdaBody to manage lambda line break inside function parameter call (in Allman style).
Jul 3 2018, 2:28 AM · Restricted Project
klimek added inline comments to D44609: [Clang-Format] New option BeforeLambdaBody to manage lambda line break inside function parameter call (in Allman style).
Jul 3 2018, 12:09 AM · Restricted Project

Jul 2 2018

klimek added a comment to D28462: clang-format: Add new style option AlignConsecutiveMacros.

@klimek having gotten that out of the way, I do occasionally drink too much and have sudden urges to re-implement things from scratch. Close it if you need to, since I can't commit to anything, but.... it it happens to be still open on one of those nights, who knows, maybe I'll end up doing it :)

Jul 2 2018, 12:45 AM · Restricted Project
klimek added inline comments to D45094: [LibTooling] Make interface of VFS injection into ClangTool more user-friendly.
Jul 2 2018, 12:44 AM · Restricted Project
klimek accepted D48159: [clangd] Implement hover for "auto" and "decltype".
Jul 2 2018, 12:22 AM

Jun 29 2018

klimek added a reviewer for D48159: [clangd] Implement hover for "auto" and "decltype": rsmith.
Jun 29 2018, 2:47 AM
klimek added a comment to D28462: clang-format: Add new style option AlignConsecutiveMacros.

@klimek fair point. To be honest, I've pretty much lost interest / momentum on this feature, I very much doubt I will ever go back and re-implement from scratch as you suggest.
Not meaning to sound rude, I just don't want to waste anyone's time who is waiting for this (seems there are a few).

Jun 29 2018, 2:38 AM · Restricted Project

Jun 28 2018

klimek added inline comments to D48159: [clangd] Implement hover for "auto" and "decltype".
Jun 28 2018, 1:18 AM

Jun 27 2018

klimek added a comment to D44609: [Clang-Format] New option BeforeLambdaBody to manage lambda line break inside function parameter call (in Allman style).
Hello,

after my last modification (require by previous comment), I do not see any feedback or validation for this patch.
Is their something special to do in order to go forward on this patch?

Lambda are more an more used in modern C++, and it's very annoying to not have a way to format them in allman style.

Jun 27 2018, 12:35 AM · Restricted Project

Jun 26 2018

klimek accepted D45719: [clang-Format] Fix indentation of member call after block.

LG.

Jun 26 2018, 4:56 AM
klimek added inline comments to D45719: [clang-Format] Fix indentation of member call after block.
Jun 26 2018, 1:12 AM

Jun 25 2018

klimek added inline comments to D45719: [clang-Format] Fix indentation of member call after block.
Jun 25 2018, 8:54 AM
klimek added inline comments to D45719: [clang-Format] Fix indentation of member call after block.
Jun 25 2018, 4:30 AM

Jun 21 2018

klimek added inline comments to D48259: [clang-format] Fix bug with UT_Always when there is less than one full tab.
Jun 21 2018, 6:56 AM · Restricted Project, Restricted Project

Jun 18 2018

klimek accepted D48242: [ASTMatchers] Add support for matching the type of a friend decl..

lg, thx

Jun 18 2018, 2:12 AM · Restricted Project
klimek accepted D48269: [ASTMatchers] Don't assert-fail in specifiesTypeLoc()..

lg, thanks!

Jun 18 2018, 1:59 AM · Restricted Project

Jun 17 2018

klimek added inline comments to D48259: [clang-format] Fix bug with UT_Always when there is less than one full tab.
Jun 17 2018, 11:11 PM · Restricted Project, Restricted Project

Jun 15 2018

klimek added inline comments to D48242: [ASTMatchers] Add support for matching the type of a friend decl..
Jun 15 2018, 5:02 PM · Restricted Project
klimek added a comment to D48230: [PredicateInfo] Order instructions in different BBs by DFSNumIn..

Florian: did you manually add Danny on this (the account is disabled and I'm trying to figure out where / how folks are added).

Jun 15 2018, 4:52 PM

Jun 14 2018

klimek accepted D47759: [Format] Do not use a global static value for EOF within ScopedMacroState..

LG.

Jun 14 2018, 9:04 PM · Restricted Project

Jun 12 2018

klimek accepted D46024: [clang-format] Add SpaceBeforeCpp11BracedList option..

FWIW, please note that this space-before-brace style is not specific to WebKit; CppCoreGuidelines exhibits it as well:
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es23-prefer-the--initializer-syntax

This and WebKit's style seem like compelling arguments to support this option.

klimek, djasper: Do you have any objections against landing this?

Jun 12 2018, 10:27 AM

May 30 2018

klimek accepted D47095: [clang-format/ObjC] Correctly parse Objective-C methods with 'class' in name.

LG

May 30 2018, 2:17 AM

May 9 2018

klimek committed rCTE331875: Do not warn on unused parameters for functions with empty bodies..
Do not warn on unused parameters for functions with empty bodies.
May 9 2018, 6:23 AM
klimek committed rL331875: Do not warn on unused parameters for functions with empty bodies..
Do not warn on unused parameters for functions with empty bodies.
May 9 2018, 6:23 AM

May 2 2018

klimek added a comment to D36610: [Tooling] Add option to getFullyQualifiedName using a custom PritingPolicy.

I believe this was accepted by rnk - are you waiting for specific further feedback?

May 2 2018, 12:54 AM

Apr 26 2018

klimek added a comment to D46024: [clang-format] Add SpaceBeforeCpp11BracedList option..

Is this written down somewhere? https://webkit.org/code-style-guidelines/ doesn't seem to mention it.

Apr 26 2018, 6:01 AM
klimek added inline comments to D44609: [Clang-Format] New option BeforeLambdaBody to manage lambda line break inside function parameter call (in Allman style).
Apr 26 2018, 6:01 AM · Restricted Project

Apr 25 2018

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

LG

Apr 25 2018, 7:42 AM
klimek added inline comments to D45719: [clang-Format] Fix indentation of member call after block.
Apr 25 2018, 3:27 AM

Apr 23 2018

klimek committed rCTE330580: Fix tests after changes to clang-format in r330573..
Fix tests after changes to clang-format in r330573.
Apr 23 2018, 4:51 AM
klimek committed rL330580: Fix tests after changes to clang-format in r330573..
Fix tests after changes to clang-format in r330573.
Apr 23 2018, 4:51 AM
klimek committed rL330573: Format closing braces when reformatting the line containing the opening brace..
Format closing braces when reformatting the line containing the opening brace.
Apr 23 2018, 2:37 AM
klimek committed rC330573: Format closing braces when reformatting the line containing the opening brace..
Format closing braces when reformatting the line containing the opening brace.
Apr 23 2018, 2:37 AM
klimek closed D45726: Format closing braces when reformatting the line containing theopening brace..
Apr 23 2018, 2:37 AM

Apr 20 2018

klimek added a comment to D45726: Format closing braces when reformatting the line containing theopening brace..

Another point: for the example in the summary about bailing-out early, is there a test for this already? If not, we should add one.

Apr 20 2018, 3:04 AM
klimek updated the diff for D45726: Format closing braces when reformatting the line containing theopening brace..

Address comments.

Apr 20 2018, 3:04 AM
klimek added inline comments to D28462: clang-format: Add new style option AlignConsecutiveMacros.
Apr 20 2018, 1:05 AM · Restricted Project

Apr 17 2018

klimek created D45726: Format closing braces when reformatting the line containing theopening brace..
Apr 17 2018, 9:10 AM
klimek accepted D38615: [libclang] Only mark CXCursors for explicit attributes with a type.

lg

Apr 17 2018, 2:31 AM

Apr 11 2018

klimek committed rL329816: Fix bugs around handling C++11 attributes..
Fix bugs around handling C++11 attributes.
Apr 11 2018, 7:55 AM
klimek committed rC329816: Fix bugs around handling C++11 attributes..
Fix bugs around handling C++11 attributes.
Apr 11 2018, 7:55 AM

Apr 9 2018

klimek accepted D43969: Improve completion experience for headers.

lg

Apr 9 2018, 6:29 AM

Jan 16 2018

klimek added a comment to D42036: [clang-format] Keep comments aligned to macros.

Just from a formatting point of view, why not:

//.   Comment
#.    define X
Jan 16 2018, 1:16 AM

Jan 5 2018

klimek added inline comments to D41487: [clang-format] Adds a FormatStyleSet.
Jan 5 2018, 7:07 AM

Dec 21 2017

klimek added inline comments to D41487: [clang-format] Adds a FormatStyleSet.
Dec 21 2017, 7:06 AM

Dec 18 2017

klimek added a comment to D37813: clang-format: better handle namespace macros.
In D37813#958116, @Typz wrote:

OK.

So you mean a solution like the one discussed earlier would be the way to go?

I mean that we can configure macros in the format style, like "define A(X) class X {". I'm not 100% sure whether we would just try to use the Preprocessor for this, or whether we'd want to only allow a small subset of actual macros, but the general idea would be the same: The UnwrappedLineParser would parse the macro at the expansion location A(X) into an unwrapped line, and then parse the expansion into a child line, with the tokens tha tare not in the argument of the call being marked as fixed (parent child might also be better inverted).

(As a side-note, I want to stress out that we would actually need a 'reversible' description to support the namespace case, to allow generating the end-of-namespace comment)

Dec 18 2017, 4:23 AM