Page MenuHomePhabricator

klimek (Manuel Klimek)
UserAdministrator

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Wed, Jul 29

klimek added inline comments to D83296: [clang-format] Add a MacroExpander..
Wed, Jul 29, 8:37 AM · Restricted Project, Restricted Project
klimek updated the diff for D83296: [clang-format] Add a MacroExpander..

Addressed code review comments.

Wed, Jul 29, 8:34 AM · Restricted Project, Restricted Project

Jul 13 2020

klimek added a comment to D83296: [clang-format] Add a MacroExpander..

Monday-morning ping.

Thanks for the reminder here... however this is taking me a bit to get my head around, and we've got a release branch cut scheduled for a couple of days that we're trying to polish for.
AFAICT there's significant followup work still needed to make use of this - are you wanting this to land in the 11 release? Else i'd probably come back to this after the cut...

Jul 13 2020, 4:15 AM · Restricted Project, Restricted Project
klimek added a comment to D83296: [clang-format] Add a MacroExpander..

Monday-morning ping.

Jul 13 2020, 3:03 AM · Restricted Project, Restricted Project
klimek updated subscribers of D83621: [clang][Tooling] Try to avoid file system access if there is no record for the file in compile_commads.json.

@djasper wrote this iirc, but I doubt he'll have much time to look into this :)

Jul 13 2020, 2:04 AM · Restricted Project

Jul 9 2020

klimek added inline comments to D79773: [clang-format] Improve clang-formats handling of concepts.
Jul 9 2020, 4:32 AM · Restricted Project, Restricted Project

Jul 7 2020

Herald added a project to D83296: [clang-format] Add a MacroExpander.: Restricted Project.
Jul 7 2020, 4:54 AM · Restricted Project, Restricted Project
klimek committed rG8c2a61397607: Hand Allocator and IdentifierTable into FormatTokenLexer. (authored by klimek).
Hand Allocator and IdentifierTable into FormatTokenLexer.
Jul 7 2020, 3:01 AM
klimek closed D83218: Hand Allocator and IdentifierTable into FormatTokenLexer..
Jul 7 2020, 3:00 AM · Restricted Project
klimek updated the diff for D83218: Hand Allocator and IdentifierTable into FormatTokenLexer..

Address review comment.

Jul 7 2020, 2:53 AM · Restricted Project

Jul 6 2020

Herald added a project to D83218: Hand Allocator and IdentifierTable into FormatTokenLexer.: Restricted Project.
Jul 6 2020, 5:46 AM · Restricted Project

Jul 3 2020

klimek accepted D83076: Revert AST Matchers default to AsIs mode.

lg

Jul 3 2020, 1:03 AM · Restricted Project

Jun 30 2020

klimek accepted D82771: [ASTMatcher] Fix a performance regression: memorize the child match..

LG

Jun 30 2020, 5:23 AM · Restricted Project
klimek added a comment to D82771: [ASTMatcher] Fix a performance regression: memorize the child match..

In what situation are we calling child matchers repeatedly with the same matcher on the same node?

I guess a pattern like https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp#L29-L40?

Jun 30 2020, 1:35 AM · Restricted Project

Jun 29 2020

klimek added inline comments to D82771: [ASTMatcher] Fix a performance regression: memorize the child match..
Jun 29 2020, 8:37 AM · Restricted Project
klimek added a comment to D82771: [ASTMatcher] Fix a performance regression: memorize the child match..

In what situation are we calling child matchers repeatedly with the same matcher on the same node?

Jun 29 2020, 8:36 AM · Restricted Project

Jun 22 2020

klimek added a reviewer for D80202: [ASTMatchers] Performance optimization for memoization: sammccall.

+Sam for additional sanity checking.

Jun 22 2020, 8:35 AM · Restricted Project
klimek added a comment to D78867: [docs] avoid 'arc land' command.

General note on that patch:
I am fine with both views, I know people who like to use arc land, I know people (like myself), who like to not have noisy messages.
I don't know that we have a good process to really decide that (Chris as BDFL dropped himself out of this one :)
I think giving folks a arcfilter line like Mehdi did is a great idea if we decide to go with the policy.

Jun 22 2020, 2:38 AM · Restricted Project

Jun 18 2020

klimek added a comment to D80961: Ignore template instantiations if not in AsIs mode.
  1. the scare quotes around "standing objections" reads like you're not respecting the opinions of others here;

Hmm, this wasn't intended. I sometimes quote things if they are a particular term (ordinarily I would quote "particular term").

Nevertheless, I'll take on your message. I don't have more to say.

Jun 18 2020, 2:08 AM · Restricted Project

Jun 16 2020

klimek added a comment to D80961: Ignore template instantiations if not in AsIs mode.

Without jumping into the discussion whether it should be the default, I think we should be able to control template instantiation visitation separately from other implicit nodes.
Having to put AsIs on a matcher every time you need to match template instantiations is a rather big change (suddenly you have to change all the matchers you've written so far).

I think that's the intended meaning of AsIs though. Template instantiations are not source code the user wrote, they're source code the compiler stamped out from code the user wrote. I hope IgnoreUnlessSpelledInSource isn't a misnomer.

I love the idea of being able to control visitation of template instantiation.
I am somewhat torn on whether it should be the default, and would like to see more data.
I feel more strongly about needing AsIs when I want to match template instantiations.

FWIW, my experience in clang-tidy has been that template instantiations are ignored far more often than they're desired. In fact, instantiations tend to be a source of bugs for us because they're easy to forget about when writing matchers without keeping templates in mind. The times when template instantiations become important to *not* ignore within the checks is when the check is specific to template behavior, but that's a minority of the public checks thus far.

So I assume the idea is that this will work & be what we'll want people to write?
traverse(AsIs, decl(traverse(IgnoreImplicit, hasFoo)))

I believe so, yes. It's explicit about which traversal mode you want any given set of matchers to match with, so it is more flexible at the expense of being more verbose. Alternatively, you could be in AsIs mode for all of it and explicitly ignore the implicit nodes you wish to ignore (which may be even more verbose with the same results, depending on the matchers involved).

Jun 16 2020, 4:23 AM · Restricted Project
klimek accepted D80025: [ASTMatcher] Correct memoization bug ignoring direction (descendants or ancestors).

LG. Thanks!

Jun 16 2020, 3:17 AM · Restricted Project

Jun 15 2020

klimek added a comment to D81552: [ASTMatchers] Added hasDirectBase Matcher.

We have some precedent for overloading has* matchers, but I'll defer to Sam's judgement whether that's a good idea here.

Jun 15 2020, 2:08 AM · Restricted Project

Jun 8 2020

klimek added a comment to D80721: Updates to the 'CLion Integration' section in ClangFormat docs.

@sammccall, @klimek, @gribozavr2, @MyDeveloperDay who would be the best reviewer for this?
Could do the reviews myself for small CLion-related documentation changes if everyone is ok with this. Does that LG?

Jun 8 2020, 8:11 AM · Restricted Project
klimek added a comment to D80961: Ignore template instantiations if not in AsIs mode.

Without jumping into the discussion whether it should be the default, I think we should be able to control template instantiation visitation separately from other implicit nodes.
Having to put AsIs on a matcher every time you need to match template instantiations is a rather big change (suddenly you have to change all the matchers you've written so far).

I think that's the intended meaning of AsIs though. Template instantiations are not source code the user wrote, they're source code the compiler stamped out from code the user wrote. I hope IgnoreUnlessSpelledInSource isn't a misnomer.

I love the idea of being able to control visitation of template instantiation.
I am somewhat torn on whether it should be the default, and would like to see more data.
I feel more strongly about needing AsIs when I want to match template instantiations.

FWIW, my experience in clang-tidy has been that template instantiations are ignored far more often than they're desired. In fact, instantiations tend to be a source of bugs for us because they're easy to forget about when writing matchers without keeping templates in mind. The times when template instantiations become important to *not* ignore within the checks is when the check is specific to template behavior, but that's a minority of the public checks thus far.

Jun 8 2020, 2:08 AM · Restricted Project

Jun 4 2020

klimek added a comment to D80961: Ignore template instantiations if not in AsIs mode.

Without jumping into the discussion whether it should be the default, I think we should be able to control template instantiation visitation separately from other implicit nodes.
Having to put AsIs on a matcher every time you need to match template instantiations is a rather big change (suddenly you have to change all the matchers you've written so far).
I love the idea of being able to control visitation of template instantiation.
I am somewhat torn on whether it should be the default, and would like to see more data.
I feel more strongly about needing AsIs when I want to match template instantiations.

Jun 4 2020, 3:13 AM · Restricted Project
klimek added a comment to D80961: Ignore template instantiations if not in AsIs mode.

Thank you for bringing up this issue. I think it highlights an underlying problem -- editing templates is quite difficult -- that neither setting will address, as Dmitri expanded on above. Given the parallel to macros, I'd say your change is better than the status quo. Most clang tidies and other rewriting tools that I've encountered simply skip code in macro expansions, rather than reason about how to update the macro definition or whatnot. So, by that reasoning, we should skip template instantations.

Jun 4 2020, 3:13 AM · Restricted Project

May 27 2020

klimek added a comment to D50078: clang-format: support aligned nested conditionals formatting.

For the policy question: clang-format does intentionally not try to be stable - the "how to" suggestion for clang-format has always been to format changes lines and live with divergence (divergence from people manually formatting things is larger).

May 27 2020, 1:02 AM · Restricted Project, Restricted Project

May 25 2020

klimek added inline comments to D69764: [clang-format] Add East/West Const fixer capability.
May 25 2020, 4:46 AM · Restricted Project, Restricted Project

May 20 2020

klimek added inline comments to D80025: [ASTMatcher] Correct memoization bug ignoring direction (descendants or ancestors).
May 20 2020, 8:10 AM · Restricted Project
klimek added a comment to D80202: [ASTMatchers] Performance optimization for memoization.

Given the extra complexity I'd like to see that it matters - bound nodes tend to be small.

I put that in the description, but this is where i need help. whats the best way to benchmark the matchers?
Also, do you know how it was benchmarked when MaxMemoizationEntries was decided upon?
There was also comments about some making some micro benchmarks but I don't think that was acted upon.

May 20 2020, 4:18 AM · Restricted Project
klimek added a comment to D80202: [ASTMatchers] Performance optimization for memoization.

Given the extra complexity I'd like to see that it matters - bound nodes tend to be small.

May 20 2020, 3:12 AM · Restricted Project

May 19 2020

klimek added inline comments to D80025: [ASTMatcher] Correct memoization bug ignoring direction (descendants or ancestors).
May 19 2020, 1:03 AM · Restricted Project

May 18 2020

klimek added inline comments to D80025: [ASTMatcher] Correct memoization bug ignoring direction (descendants or ancestors).
May 18 2020, 3:10 AM · Restricted Project

May 13 2020

klimek committed rG53cc90f78999: Make FormatToken::Type private. (authored by klimek).
Make FormatToken::Type private.
May 13 2020, 8:06 AM
klimek closed D67405: Make FormatToken::Type private..
May 13 2020, 8:05 AM · Restricted Project
klimek updated the diff for D67405: Make FormatToken::Type private..

Update docs.

May 13 2020, 7:32 AM · Restricted Project
klimek updated the diff for D67405: Make FormatToken::Type private..

Rebase.

May 13 2020, 7:32 AM · Restricted Project

May 8 2020

klimek added a comment to D79575: Extend Compilation Database by an optional field for compiler arguments.

Adding Sam, as I remember clangd having that problem.

May 8 2020, 3:11 AM
klimek added a reviewer for D79575: Extend Compilation Database by an optional field for compiler arguments: sammccall.
May 8 2020, 3:11 AM

Jan 28 2020

klimek accepted D73537: [clangd] add CODE_OWNERS.

lg

Jan 28 2020, 3:27 AM · Restricted Project

Jan 8 2020

klimek added inline comments to D72216: [clang-format] [PR44189] Remove space between identifier and scope resolution operator unless the identifier is a macro.
Jan 8 2020, 1:02 AM · Restricted Project, Restricted Project

Dec 2 2019

klimek added a comment to D69764: [clang-format] Add East/West Const fixer capability.

I'm not generally opposed to this, given that
a) clang-format already changes code; I think by now we're not fixing double semicolon mainly for workflow reasons, would be fine to add
b) the implementation is very self contained

Dec 2 2019, 2:23 AM · Restricted Project, Restricted Project

Nov 25 2019

klimek accepted D70664: [clang-format] update string comparison in clang-format.py.

lg

Nov 25 2019, 4:32 AM · Restricted Project, Restricted Project
klimek added a comment to D70633: clang-format-vs : Fix Unicode formatting.

generally makes sense

Nov 25 2019, 1:36 AM · Restricted Project, Restricted Project

Oct 30 2019

klimek added inline comments to D69577: [clang-format] [PR35518] C++17 deduction guides are wrongly formatted.
Oct 30 2019, 9:40 AM · Restricted Project, Restricted Project, Restricted Project

Oct 24 2019

klimek added a comment to D69122: Add support to find out resource dir and add it as compilation args.

since resource directory should be picked relative
to the path of the clang-compiler in the compilation command.

Oct 24 2019, 12:18 PM · Restricted Project

Oct 23 2019

klimek accepted D69351: Improve Clang's getting involved document and make it more inclusive in wording..

lg

Oct 23 2019, 12:52 PM · Restricted Project
klimek added inline comments to D69351: Improve Clang's getting involved document and make it more inclusive in wording..
Oct 23 2019, 12:32 PM · Restricted Project

Oct 21 2019

klimek accepted D68969: [clang-format] Remove the dependency on frontend.

lg

Oct 21 2019, 8:22 PM · Restricted Project, Restricted Project

Oct 17 2019

klimek added inline comments to D68969: [clang-format] Remove the dependency on frontend.
Oct 17 2019, 1:57 AM · Restricted Project, Restricted Project

Oct 16 2019

klimek added a comment to D68969: [clang-format] Remove the dependency on frontend.

My intuitive solution would have been to get the char* for the start and end-location and then search forward and backwards for \n.

I may need to, it feels slow for large files which I suspect is in the splitting, which will be mostly unnecessary.

Oct 16 2019, 3:19 AM · Restricted Project, Restricted Project

Oct 15 2019

klimek added a comment to D68969: [clang-format] Remove the dependency on frontend.

My intuitive solution would have been to get the char* for the start and end-location and then search forward and backwards for \n.

Oct 15 2019, 4:45 AM · Restricted Project, Restricted Project
klimek added a comment to D68914: [clang-format] Remove duplciate code from Invalid BOM detection.
Oct 15 2019, 2:03 AM · Restricted Project, Restricted Project, Restricted Project

Oct 14 2019

klimek added inline comments to D68914: [clang-format] Remove duplciate code from Invalid BOM detection.
Oct 14 2019, 1:51 AM · Restricted Project, Restricted Project, Restricted Project
klimek added a comment to D68554: [clang-format] Proposal for clang-format to give compiler style warnings.
Could you or anyone else point me to a good example, I'd definitely like to take a look.

http://llvm-cs.pcc.me.uk/include/llvm/Support/SourceMgr.h/rSMDiagnostic --

`via clang-format?` did you mean via clang.exe or clang-check.exe?   I did think of this when I started doing this work, I just came to the conclusion that if I did that it wouldn't be long before someone requested the same features in clang-format itself.

Err, via clang-tidy. Sorry, that was a super confusing typo. That seems like a great place for this feature to me.

Oct 14 2019, 1:51 AM · Restricted Project, Restricted Project, Restricted Project

Oct 11 2019

klimek accepted D68554: [clang-format] Proposal for clang-format to give compiler style warnings.

lg

Oct 11 2019, 2:54 AM · Restricted Project, Restricted Project, Restricted Project
klimek added inline comments to D40221: [clang-format] Parse blocks in braced lists.
Oct 11 2019, 12:39 AM · Restricted Project
klimek committed rL374520: Add github commit access for r4nt..
Add github commit access for r4nt.
Oct 11 2019, 12:30 AM

Oct 10 2019

klimek added a comment to D68554: [clang-format] Proposal for clang-format to give compiler style warnings.

I think it's easy enough to do as a kind of driver, either in python or C++, but then again, at that point putting it into ClangFormat seems fine. One thing that I find on the one side kinda nice, but on the other side also weird is the names of the flags. Making them more similar to clang flags is nice from the point of people used to clang being able to quickly identify them, but the structure also makes it look like clang-format would support a wider range of flags, which it doesn't. Not sure which side I'm on, and I'm fine with the patch as is (minus pulling out a function), but wanted to bring it up if folks have ideas.

The reason I added -Wclang-format-violations and -Wnoclang-format-violations was not only because I wanted to treat them like compiler warnings, but also so that the command line argument could be used the turning on/off to alter the exit code so as to either stop a build or continue based on -Werror or if you'd turn the individual warning off.

Also I know there are external tools, (I think SonarCube) which I think could read the code in the [] e.g. 'ClangFormat.cpp:54:29: warning: code should be clang-formatted [-Wclang-format-violations]' and be able to use that along with the file, row and column information to categorize and the annotate source code.

if you think this is too much perhaps I should make a new tool clang-format-check that did only this, but then I am concerned that it would be almost 100% clang-format anyway, and it wouldn't be long before a request came in to merge the functionality.

Oct 10 2019, 5:03 AM · Restricted Project, Restricted Project, Restricted Project
klimek added a comment to D68554: [clang-format] Proposal for clang-format to give compiler style warnings.

As this behaviour is not something clang-format should do because there are external tool that can do this for us, I thought I'd set myself a challenge of trying to write this with external tools, to prove to myself that it was indeed easy, what I want to achieve is being able to go to a directory and determine if there are any clang-format changes needed in the source files of the levels below with a single command.

The idea would be that this could be something that could fit on the command line, or made into a bash alias/function (more likely), I really don't want to turn to python as if I'm coding this in another language I might as well be coding it in C++ inside clang-format and it opens me up to the whole can't find the correct python headache. (but I also think the clang diagnostic classes give doing it inside C++ the edge)

I want to take into account at least the concepts from D29039: [clang-format] Proposal for clang-format -r option about being able to look deeply into a directory.

There are a couple of requirements: (some nice to have's)

  1. look deeply into a directory for all files supported by clang-format (C++,C# etc)
  2. file extensions should be case insensitive i.e. .cpp or .CPP
  3. be able to clearly see the name of the file that need clang-formatting
  4. don't actually do the formatting (tell me what needs doing but don't necessarily make the change)
  5. ideally show a snippet of the file where the change is needed (I guess this isn't essential)
  6. This should be cross platform windows, linux, (I guess other *nix's) it should work in at least the common shells bash,cygwin,powershell

The cross platform is the real killer, this forces you down the python road, (or the C++) if you want a common solution otherwise you are implementing different solutions on different plaforms.

But lets ignore that and lets start with a Linux or Cygwin bash shell. Surely the best way to find all source files that can be converted is via a find. In the past I've only ever used find like this to find multiple extensions

find .\( -name "*.h" -o name "*.cpp" \) -print

But this is really unworkable for so many output formats that clang-format supports, and even if you could get that to work it would be case sensitive unless you uses -ilname which would make the command line even longer

So lets try using -iregex, this is a much shorter case insensitive command but still, it needs a certain amount of escaping in the regular expression, (lets hope we never do "clang-format all the things") as this could get longer.

find . -iregex '.*\.\(cpp\|cc\|c\+\+\|cxx\|c\|cl\|h\|hh\|hpp\|m\|mm\|inc\|js\|ts\|protor\|protodevel\|java\|cs\)' -print

if I want to put that into an alias then I need switch the ' and " around

alias find_all_code='find . -iregex ".*\.\(cpp\|cc\|c\+\+\|cxx\|c\|cl\|h\|hh\|hpp\|m\|mm\|inc\|js\|ts\|protor\|protodevel\|java\|cs\)" -print'

Is the same thing as easy in powershell? Ok not really .. this is as far as I got, and I couldn't work out how to handle multiple extensions let alone case sensitivity.. (it feels very slow too!)

PowerShell.exe -Command "dir -Recurse -ea 0 | ? FullName -like '*.cpp'

So at least my brief initial conclusion is... if we are not using C++, its Python or nothing if you want cross-platform or maybe find if you only care about *nix

But even the find... that only covers 1) and 2) of my requirements...how do I determine even with find which files need formatting..

This command would show me the contents of everything that needs changing.

find . -iregex '.*\.\(cpp\|cc\|c\+\+\|cxx\|c\|cl\|h\|hh\|hpp\|m\|mm\|inc\|js\|ts\|protor\|protodevel\|java\|cs\)' -exec clang-format {} \;

This would give me the list of changes, but then I lose the name of the file.

find . -iregex '.*\.\(cpp\|cc\|c\+\+\|cxx\|c\|cl\|h\|hh\|hpp\|m\|mm\|inc\|js\|ts\|protor\|protodevel\|java\|cs\)' -exec clang-format --output-replacements-xml {} \;

So using external tools is easy how? the dumping of the output to stdout or the replacement xml to stdout makes this much harder as I lose the filename that was passed to clang-format, (there is probably an xargs trick here I'm missing)

This proposed solution would make all this possible just with:

find . -iregex '.*\.\(cpp\|cc\|c\+\+\|cxx\|c\|cl\|h\|hh\|hpp\|m\|mm\|inc\|js\|ts\|protor\|protodevel\|java\|cs\)' -exec clang-format -i -n {} \;

and more to the point the list of file types supported by clang-format is something clang-format knows... why not combine this with D29039: [clang-format] Proposal for clang-format -r option and just do

clang-format -r -i -n .

This provides in my view the only cross platform, shell independent way of doing this in what is a relatively minimal code change, and I'm struggling to see this as easy to do with an external tools? but if someone wants to show me how, I'd be interested to learn.

Oct 10 2019, 12:20 AM · Restricted Project, Restricted Project, Restricted Project
klimek added inline comments to D68660: [tooling] Teach Tooling to understand compilation with offloading..
Oct 10 2019, 12:11 AM · Restricted Project

Oct 6 2019

klimek added a comment to D68554: [clang-format] Proposal for clang-format to give compiler style warnings.

Why not build a tool that takes the diff output of clang-format and changes it to what you want? Wouldn't that be a couple of lines of python?

Oct 6 2019, 9:23 AM · Restricted Project, Restricted Project, Restricted Project

Oct 2 2019

klimek added a comment to D68296: [clang-format] Add ability to wrap braces after multi-line control statements.

Thanks for the review. I have added documentation updates.

I do not have a public style guide to reference. My company just switched to auto-clang-formatting all of our code, and this patch addresses one of our biggest pain points: readability. When the opening brace of a multi-line control statement is not wrapped, we simply cannot tell at first glance where the control statement ends and where the body begins due to our indentation settings.

I think the fact that you yourself see the benefit of this change speaks to its viability.

I cannot think of any other brace wrapping rules that need this level of control. Our code base consists of millions of lines of code, so I'm pretty sure we would have found another instance.

I would be willing to support this patch and further revisions if this gets into master, as our company will depend on this feature.

Oct 2 2019, 8:10 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Sep 26 2019

klimek accepted D68072: [clang-format] Reference qualifiers in member templates causing extra indentation..

LG, nice patch, thanks!

Sep 26 2019, 1:12 AM · Restricted Project, Restricted Project, Restricted Project

Sep 25 2019

klimek added a comment to rG82aaf1741213: [clang-format] [PR36858] Add missing .hh and .cs extensions from python support….

@klimek I'm wondering if we should be thinking about including .ixx and .mxx files (or other common modules files names)

Sep 25 2019, 6:30 AM

Sep 24 2019

klimek accepted D67949: [clang-format] [PR36858] Add missing .hh and .cs extensions from python support utilities.

lg

Sep 24 2019, 2:26 AM · Restricted Project, Restricted Project, Restricted Project

Sep 18 2019

klimek accepted D67718: [clang-format][PR41899] PointerAlignment: Left leads to useless space in lambda intializer expression.

LG

Sep 18 2019, 10:06 AM · Restricted Project, Restricted Project, Restricted Project
klimek accepted D67670: [clang-format][PR41964] Fix crash with SIGFPE when TabWidth is set to 0 and line starts with tab.

LG. That makes lots of sense now :) Thanks!

Sep 18 2019, 12:48 AM · Restricted Project, Restricted Project, Restricted Project

Sep 17 2019

klimek added inline comments to D67670: [clang-format][PR41964] Fix crash with SIGFPE when TabWidth is set to 0 and line starts with tab.
Sep 17 2019, 11:27 AM · Restricted Project, Restricted Project, Restricted Project

Sep 16 2019

klimek accepted D67541: [ClangFormat] Future-proof Standard option, allow floating or pinning to arbitrary lang version.

LG

Sep 16 2019, 1:08 AM · Restricted Project, Restricted Project

Sep 10 2019

klimek created D67405: Make FormatToken::Type private..
Sep 10 2019, 9:08 AM · Restricted Project

Aug 20 2019

klimek accepted D66462: Removed the 'id' AST matcher, which is superseded by '.bind()'.

Yay, thanks!

Aug 20 2019, 2:52 AM · Restricted Project, Restricted Project

Jul 25 2019

klimek accepted D65227: clang-format: Support `if CONSTEXPR` if CONSTEXPR is a macro..

I am surprised this works as well as it does, but tests seem to indicate it does, so... LG (minus potentially reducing some duplication :)

Jul 25 2019, 5:18 PM · Restricted Project

Jul 24 2019

klimek added a comment to D65092: [clang] Add isDirectlyDerivedFrom AST Matcher..

So, I did like the more exhaustive doc, I thought you'd move it to the code so it also shows up in the generated doc page :) (sorry for not being clearer here)

Jul 24 2019, 6:38 AM · Restricted Project, Restricted Project

Jul 23 2019

klimek accepted D65125: clang-format: Fix namespace end comments for namespaces with attributes and macros.

LG, thx!

Jul 23 2019, 6:49 AM · Restricted Project

Jul 22 2019

klimek requested changes to D65092: [clang] Add isDirectlyDerivedFrom AST Matcher..
Jul 22 2019, 12:28 PM · Restricted Project, Restricted Project
klimek added inline comments to D65092: [clang] Add isDirectlyDerivedFrom AST Matcher..
Jul 22 2019, 10:39 AM · Restricted Project, Restricted Project

Jul 8 2019

klimek added a comment to D64297: [JSONCompilationDatabase] Strip distcc/ccache/gomacc wrappers from parsed commands..

Is distcc gcc.exe or gomacc gcc.exe possible?

The design of the compilation database/plugin model makes it pretty hard to add options.

Yes. It also lacks user settings. Downstream compile_commands.json consumers may provide their own command line option filtering mechanism that users can customize and add more compiler schedulers. For some tools, they probably don't want to see filtering applying on multiple layers, especially if the filtering mechanism done at the JSONCompilationDatabase layer may have false positives.

Jul 8 2019, 12:28 AM · Restricted Project

Jun 28 2019

klimek accepted D63734: Update CODE_OWNERS.txt for clang-doc.

LG

Jun 28 2019, 2:19 AM · Restricted Project, Restricted Project

Jun 6 2019

klimek accepted D37813: clang-format: better handle namespace macros.

LG

Jun 6 2019, 7:27 AM · Restricted Project, Restricted Project

May 27 2019

klimek added inline comments to D37813: clang-format: better handle namespace macros.
May 27 2019, 12:44 AM · Restricted Project, Restricted Project

May 23 2019

klimek added inline comments to D37813: clang-format: better handle namespace macros.
May 23 2019, 4:24 AM · Restricted Project, Restricted Project
klimek added a comment to D37813: clang-format: better handle namespace macros.

The patch goal is indeed to indent the content of namespace-macro blocks like the content of any 'regular' namespace. So it should look like the content of a namespace, possibly depending on the choose style options. To sumarize, here is a detailed summary of the observable differences between macros vs normal namespace.

Without this patch, clang-format does not understand the macro call actually defines a namespace, thus:

  • the content of the namespace-macro block is indented, even when Style.NamespaceIndentation = FormatStyle::NI_None
TESTSUITE(A) {
  int i;                  // <--- should not be indented
}
namespace B {
int j;
}
  • similarly for nested namespaces, when Style.NamespaceIndentation = FormatStyle::NI_Inner :
TESTSUITE(A) {
TESTSUITE(B) {
    int i;                // <--- should be indented 2-chars only
}
}
namespace C {
namespace D {
  int j;
}
}
  • There is no automatic fix of namespace end comment when Style.FixNamespaceComments = true
TESTSUITE(A) {
  int i;
}                         // <--- should have a namespace end comment
namespace B {
  int j;
} // namespace B
  • Multiple nested namespaces are not compacted when Style.CompactNamespaces = true
TESTSUITE(A) {
TESTSUITE(B) {             //<--- should be merged with previous line
    int i;
}
}                         // <--- should be merged with previous line (and have a "combined" namespace end comment)
namespace A { namespace B {
int j;
} // namespace A::B

This patch fixes all these points, which hopefully leads to the exact same formatting when using the namespace keyword or the namespace-macros:

// Style.NamespaceIndentation = FormatStyle::NI_None
TESTSUITE(A) {
int i;
}
namespace B {
int j;
}

// Style.NamespaceIndentation = FormatStyle::NI_Inner
TESTSUITE(A) {
TESTSUITE(B) {
  int i;
}
}
namespace C {
namespace D {
  int j;
}
}

// Style.FixNamespaceComments = true
TESTSUITE(A) {
  int i;
} // TESTSUITE(A)
namespace B {
  int j;
} // namespace B

// Style.CompactNamespaces = true
TESTSUITE(A) { TESTSUITE(B) {
  int i;
}} // TESTSUITE(A::B)
namespace A { namespace B {
  int j;
}} // namespace A::B

I did not see any issue in my testing or reviewing the code, but if you see a place in the tests where it does not match this, please point it out !

May 23 2019, 2:03 AM · Restricted Project, Restricted Project

May 20 2019

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

I thought we had the discussion upthread that they indent differently from namespaces. I'm working on a patch that allows you to configure macros.

The current behavior is that they indent differently from namespace, because clang does not know these macros are conceptually equivalent to namespaces. And this patch actually fixes that, letting clang know they are actually macros.

May 20 2019, 7:48 AM · Restricted Project, Restricted Project

May 8 2019

klimek added inline comments to D41911: [clangd] Include scanner that finds compile commands for headers..
May 8 2019, 10:40 PM · Restricted Project

Apr 13 2019

klimek accepted D59746: [CommandLineParser] Add DefaultOption flag.

lg

Apr 13 2019, 8:16 AM · Restricted Project, Restricted Project

Apr 12 2019

klimek added inline comments to D59746: [CommandLineParser] Add DefaultOption flag.
Apr 12 2019, 1:47 AM · Restricted Project, Restricted Project

Apr 11 2019

klimek added a comment to D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range.

I agree and would be happy with the change if it would only change the line-filtered workflow, but this afaict (unless I'm missing something :) will also affect the workflow where the provided range is 0-length range, which has an implicit "format stuff around this" request from the user inside it. I'd be happy with a patch that differentiates these two sides.

This use case needs capturing in a unit test..it isn't obvious

Apr 11 2019, 9:26 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.

Thanks for the explanation! I do understand your philosophy on this, and agree with the desired behavior case you brought up where you have put in new braces.

After thinking about this more, the thing I really care about is that clang-format is idempotent with a line filter - i.e., running it twice should always have the same effect as running it once.

So, either this fix, or your proposed fix of fixing all lines until the next correct indentation would meet that idempotence criteria.

However, I think in this particular case I still prefer my fix - to me, line filter is meant to limit the effect of clang-format to just fix a particular change and the fallout from that. However, if the lines _after_ a change were wrong before, this feels very unrelated to the change that was made - why is now the time to fix these unrelated lines?

Apr 11 2019, 8:43 AM · Restricted Project, Restricted Project

Apr 9 2019

klimek added a comment to D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range.

klimek: I'm sorry, I don't fully understand your proposed fix. Could you explain it in more detail?

Without being able to respond to your proposal in detail, I strongly believe if you pass in a line range to clang-format, it should not change lines outside that range OR we should modify the documentation to clearly explain what the line filter option does.

Apr 9 2019, 8:39 AM · Restricted Project, Restricted Project
klimek added a comment to D59746: [CommandLineParser] Add DefaultOption flag.

If we make it an alias by default, can somebody overwrite that?

Unfortunately, that produces a runtime error:

lan1:/Users/dhinton/projects/llvm_project/monorepo/build/Debug $ bin/llvm-objdump -h
: CommandLine Error: Option 'h' registered more than once!
LLVM ERROR: inconsistency in registered CommandLine options

The operative lines:

llvm/tools/llvm-objdump/llvm-objdump.cpp:187:static cl::alias SectionHeadersShorter("h",
llvm/lib/Support/CommandLine.cpp:2149:static cl::alias HOpA("h", cl::desc("Alias for -help"), cl::aliasopt(HOp));

The other problem is that these are statics, so you can't count on the order, i.e., did the user overwrite get processed before or after the one in CommandLine.cpp?

Apr 9 2019, 6:02 AM · Restricted Project, Restricted Project
klimek added inline comments to D60225: [clang-format] [PR19056] Add support for indenting class members and methods one level under the modifiers.
Apr 9 2019, 1:08 AM · Restricted Project
klimek added inline comments to D59408: [clang-format] [PR25010] Extend AllowShortIfStatementsOnASingleLine not working if an "else" statement is present.
Apr 9 2019, 12:48 AM · Restricted Project
klimek added inline comments to D59332: [clang-format] AlignConsecutiveDeclarations fails with attributes.
Apr 9 2019, 12:39 AM · Restricted Project

Apr 8 2019

klimek added inline comments to D60362: [clang-format] [PR39719] clang-format converting object-like macro to function-like macro.
Apr 8 2019, 8:14 AM · Restricted Project
klimek added a comment to D59746: [CommandLineParser] Add DefaultOption flag.

A better alternative would have been to add a cl::aliasopt for '-h' in llvm's CommandLineParser when '-help' was first added. However, that's no longer possible since some llvm based tools already use '-h' for other purposes.

Is that intentional? Can you point at samples?

A quick grep found these -- there could be more.

llvm-opt-report adds -h for help and handles it after ParseCommandLineOptions returns.
llvm-objdump adds -h as an alias for --section-headers.
dsymutil adds -h for help and handles it after ParseCommandLineOptions returns.
llvm-readobj adds -h as an alias for --file-headers.
llvm-dwarfdump adds -h for help and handles it after ParseCommandLineOptions returns.
llvm-mt rolls its own via llvm-tblgen.

So, the only ones I found that aren't essentially aliases for help are in llvm-objdump and llvm-readobj.

Apr 8 2019, 8:07 AM · Restricted Project, Restricted Project
klimek added inline comments to D59087: [clang-format] [PR25010] AllowShortIfStatementsOnASingleLine not working if an "else" statement is present.
Apr 8 2019, 3:24 AM · Restricted Project, Restricted Project
klimek added inline comments to D60362: [clang-format] [PR39719] clang-format converting object-like macro to function-like macro.
Apr 8 2019, 2:04 AM · Restricted Project
klimek added a comment to D59746: [CommandLineParser] Add DefaultOption flag.

A better alternative would have been to add a cl::aliasopt for '-h' in llvm's CommandLineParser when '-help' was first added. However, that's no longer possible since some llvm based tools already use '-h' for other purposes.

Apr 8 2019, 1:58 AM · Restricted Project, Restricted Project
klimek added inline comments to D59087: [clang-format] [PR25010] AllowShortIfStatementsOnASingleLine not working if an "else" statement is present.
Apr 8 2019, 1:58 AM · Restricted Project, Restricted Project