alexfh (Alexander Kornienko)
User

Projects

User does not belong to any projects.

User Details

User Since
Jul 10 2012, 10:35 AM (306 w, 1 d)

Recent Activity

Thu, May 17

alexfh added a comment to D45177: CStringChecker, check strlcpy/strlcat.

I was unable to reproduce both FreeBSD and Linux. Plus my changes come after checkNonNull.

Thu, May 17, 3:23 PM
alexfh added a comment to D45177: CStringChecker, check strlcpy/strlcat.

See https://bugs.llvm.org/show_bug.cgi?id=37503 for a test case.

Thu, May 17, 8:31 AM
alexfh committed rCTE332609: [clang-tidy] Add a flag to enable alpha checkers.
[clang-tidy] Add a flag to enable alpha checkers
Thu, May 17, 7:08 AM
alexfh committed rL332609: [clang-tidy] Add a flag to enable alpha checkers.
[clang-tidy] Add a flag to enable alpha checkers
Thu, May 17, 7:08 AM
alexfh closed D46159: [clang-tidy] Add a flag to enable alpha checkers.
Thu, May 17, 7:08 AM
alexfh added a comment to D45177: CStringChecker, check strlcpy/strlcat.

This is reproducible in r332425.

Thu, May 17, 4:27 AM
alexfh added a comment to D45177: CStringChecker, check strlcpy/strlcat.

This patch seems to cause an assertion failure:

Thu, May 17, 4:24 AM

Wed, May 16

alexfh accepted D46980: [ASTMatchers] Introduce a blockDecl matcher for matching block declarations.

LG

Wed, May 16, 3:24 PM
alexfh added a comment to D46602: [clang-tidy] Store checks profiling info as YAML files.

A couple of comments from a cursory look. I'll try to look closer later this week.

Wed, May 16, 9:50 AM · Restricted Project
alexfh accepted D46659: [clang-tidy/google-readability-casting] Disable check for Objective-C++.

LG

Wed, May 16, 9:39 AM
alexfh accepted D46936: [Timers] TimerGroup::printJSONValues(): print mem timer with .mem suffix.

LG

Wed, May 16, 9:36 AM
alexfh accepted D46938: [Timers] TimerGroup: make printJSONValues() method public.

LG

Wed, May 16, 9:33 AM
alexfh accepted D46939: [Timers] TimerGroup: add constructor from StringMap<TimeRecord>.

LG

Wed, May 16, 9:31 AM
alexfh added a comment to D33440: clang-format: better handle statement macros.

There are still outstanding comments.

Wed, May 16, 9:27 AM
alexfh created D46951: [clang-tidy] misc-unused-parameters - retain old behavior under StrictMode.
Wed, May 16, 8:31 AM

Mon, May 14

alexfh added a comment to D46602: [clang-tidy] Store checks profiling info as YAML files.

Roman, it looks to me that a simpler storage scheme would be sufficient. For example, YYYYMMDDhhmmss-InputFileName.cpp.csv.
Main things are:

  1. include a timestamp, so there's no need to overwrite old results,

Of the input source file?

No, current timestamp, when each profile gets dumped.

Ah.
Then i don't understand this at all. Why would we want to do that, exactly?
Just so that we can avoid creating directory structure? Why do we want to avoid that?

If clang-tidy is invoked manually, a simpler naming scheme with less configuration options would be easier to use, in particular:

  1. no the need to specify the -store-check-profile-elide-prefix= option;
  2. it's easier to see all results (no need to use find, just ls /output/directory or ls /output/directory/20180514* to see today's results, for example);
  3. no chance of filename collision, and thus no chance of losing older results by just running clang-tidy again.

How do i reflect that in tests? The output name will basically be random.

Mon, May 14, 9:24 AM · Restricted Project
alexfh added a comment to D46159: [clang-tidy] Add a flag to enable alpha checkers.

Is someone able to merge in my changes?

Will do.

Mon, May 14, 8:34 AM
alexfh added a comment to D46159: [clang-tidy] Add a flag to enable alpha checkers.

Is someone able to merge in my changes?

Mon, May 14, 8:32 AM
alexfh requested changes to D46602: [clang-tidy] Store checks profiling info as YAML files.

Roman, it looks to me that a simpler storage scheme would be sufficient. For example, YYYYMMDDhhmmss-InputFileName.cpp.csv.
Main things are:

  1. include a timestamp, so there's no need to overwrite old results,

Of the input source file?

No, current timestamp, when each profile gets dumped.

Ah.
Then i don't understand this at all. Why would we want to do that, exactly?
Just so that we can avoid creating directory structure? Why do we want to avoid that?

Mon, May 14, 7:20 AM · Restricted Project

Fri, May 11

alexfh added a comment to D46602: [clang-tidy] Store checks profiling info as YAML files.

Roman, it looks to me that a simpler storage scheme would be sufficient. For example, YYYYMMDDhhmmss-InputFileName.cpp.csv.
Main things are:

  1. include a timestamp, so there's no need to overwrite old results,

Of the input source file?

Fri, May 11, 7:34 AM · Restricted Project
alexfh accepted D33844: [clang-tidy] terminating continue check.

Which comments do you think are still relevant?

Fri, May 11, 7:30 AM · Restricted Project

Wed, May 9

alexfh added inline comments to D45927: [clang-tidy] [modernize-use-auto] Correct way to calculate a type name length for multi-token types.
Wed, May 9, 8:00 AM · Restricted Project
alexfh added a comment to D46603: [Support] TimerGroup changes.

I wonder why JSON? What kind of a tool do folks use to process the outputs of printJSONValue? Is there anything existing or is JSON used "just in case"? I personally use either spreadsheets, python or shell when I deal with this kind of data, and all three of them can work reasonably well with CSV (plus the data is pretty simple).

Wed, May 9, 7:44 AM
alexfh added a comment to D46602: [clang-tidy] Store checks profiling info as YAML files.

Roman, it looks to me that a simpler storage scheme would be sufficient. For example, YYYYMMDDhhmmss-InputFileName.cpp.csv. Main things are: 1. include a timestamp, so there's no need to overwrite old results, 2. include just the name of the file without any parent directories, 3. put all outputs into the same directory. This way we wouldn't have to create a directory structure and think about stripping a certain prefix (btw, utilities like patch just specify the number of path components to remove from the start, not the actual substring). WDYT?

Wed, May 9, 7:34 AM · Restricted Project
alexfh added a comment to D46602: [clang-tidy] Store checks profiling info as YAML files.
In D46602#1092111, @rja wrote:

+1 for JSON

Wed, May 9, 7:22 AM · Restricted Project
alexfh committed rC331870: Fixes issue introduced by r331556..
Fixes issue introduced by r331556.
Wed, May 9, 5:31 AM
alexfh committed rL331870: Fixes issue introduced by r331556..
Fixes issue introduced by r331556.
Wed, May 9, 5:31 AM
alexfh closed D46633: [analyzer] add range check for InitList lookup.
Wed, May 9, 5:31 AM
alexfh accepted D46633: [analyzer] add range check for InitList lookup.

Thank you for the fix!
LG

Wed, May 9, 3:10 AM

Tue, May 8

alexfh added a comment to D46187: [Analyzer] getRegisteredCheckers(): handle debug checkers too..
In D46187#1088722, @NoQ wrote:

It seems that you're using debug checkers of the analyzer to gain some free tools for exploring the source code (such as displaying a call graph) for free, right?

I believe we could also benefit from a method of extracting the analyzer's clang -cc1 run-line from clang-tidy.

Tue, May 8, 6:58 AM
alexfh added a comment to D46188: [clang-tidy] Add a flag to enable debug checkers.

I don't think debug CSA checkers belong to clang-tidy. If one needs to dump CFG, they are probably doing quite involved stuff already, so going a step further and running clang -cc1 -analyze would not be difficult. The motivation for exposing alpha checkers (letting users test them and provide feedback) doesn't work for debug checkers.

There is word play here.
They are debug CSA checkers. But not all of them are debugging the CSA. Many of them are just utility checkers that could be just as easily a clang-tidy debug/utility checkers:

Tue, May 8, 6:56 AM
alexfh requested changes to D46188: [clang-tidy] Add a flag to enable debug checkers.

I don't think debug CSA checkers belong to clang-tidy. If one needs to dump CFG, they are probably doing quite involved stuff already, so going a step further and running clang -cc1 -analyze would not be difficult. The motivation for exposing alpha checkers (letting users test them and provide feedback) doesn't work for debug checkers.

Tue, May 8, 5:18 AM
alexfh accepted D46504: [clang-tidy] Profile is a per-AST (per-TU) data..

LG with a couple of nits.

Tue, May 8, 5:10 AM · Restricted Project
alexfh accepted D46233: [ASTMatchers] Overload isConstexpr for ifStmts.

LG

Tue, May 8, 4:39 AM
alexfh accepted D46159: [clang-tidy] Add a flag to enable alpha checkers.

Given Artem's answer (and if there are no objections from other CSA maintainers), I have no concerns with this patch going in. A couple of minor nits.

Tue, May 8, 4:36 AM

Fri, May 4

alexfh requested changes to D46159: [clang-tidy] Add a flag to enable alpha checkers.
Fri, May 4, 10:54 AM
alexfh requested changes to D45931: [ASTMatchers] Don't garble the profiling output when multiple TU's are processed.
Fri, May 4, 10:53 AM
alexfh added a comment to D46159: [clang-tidy] Add a flag to enable alpha checkers.

If the static analyzer people desire this feature, that would sway my position on it, but it sounds like they're hesitant as well.
However, I don't think clang-tidy is beholden either -- if we don't think this functionality should be exposed and can justify that position, that should carry weight as well. \
From a policy perspective, I would be fine with a policy for clang-tidy where we never expose an alpha checker from the static analyzer (or only expose checks on a case by case basis) because I don't mind users having to jump through hoops to get to experimental, unsupported functionality.

Fri, May 4, 10:51 AM
alexfh requested changes to D40937: [clang-tidy] Infinite loop checker.

Could you run the check on llvm sources and post a summary of results here?

Fri, May 4, 10:06 AM · Restricted Project
alexfh committed rC331520: Remove explicit cfg-temporary-dtors=true.
Remove explicit cfg-temporary-dtors=true
Fri, May 4, 7:16 AM
alexfh committed rL331520: Remove explicit cfg-temporary-dtors=true.
Remove explicit cfg-temporary-dtors=true
Fri, May 4, 7:16 AM
alexfh closed D46393: Remove explicit cfg-temporary-dtors=true.
Fri, May 4, 7:16 AM
alexfh added a comment to D46393: Remove explicit cfg-temporary-dtors=true.
In D46393#1086887, @NoQ wrote:

Thanks!

Just curious - did these flags bother you? Cause we never really care about cleaning up run lines after flipping the flag, so we have a lot of such stale flags in our tests. We could start cleaning them up if they cause problems.

Fri, May 4, 7:15 AM
alexfh requested changes to D33537: [clang-tidy] Exception Escape Checker.

Thank you for the updates. A few more comments.

Fri, May 4, 7:10 AM · Restricted Project
alexfh added a comment to D45931: [ASTMatchers] Don't garble the profiling output when multiple TU's are processed.

Thank you for looking at this.

From a user's perspective I'd probably prefer a different behavior of checks profiling with multiple translation units: per-file table after each file and an aggregate table at the end.

Is this a review note, or a general observation?

Why not both? ;)

BTW, that did not answer the question:

I'm sure it could be done, i'm just not too sure how useful it would be, since it seems no one before now even noticed that timing with multiple TU's was 'broken'.

Fri, May 4, 5:31 AM

Thu, May 3

alexfh accepted D46325: [clang-tidy] Define __clang_analyzer__ macro for clang-tidy for compatibility with clang static analyzer. 2nd try..

LG. Thank you!

Thu, May 3, 11:17 AM · Restricted Project
alexfh added a comment to D45931: [ASTMatchers] Don't garble the profiling output when multiple TU's are processed.

Thank you for looking at this.

From a user's perspective I'd probably prefer a different behavior of checks profiling with multiple translation units: per-file table after each file and an aggregate table at the end.

Is this a review note, or a general observation?

Thu, May 3, 10:06 AM
alexfh removed reviewers for D45931: [ASTMatchers] Don't garble the profiling output when multiple TU's are processed: klimek, bkramer, djasper, pcc.
Thu, May 3, 9:55 AM
alexfh added a comment to D46187: [Analyzer] getRegisteredCheckers(): handle debug checkers too..

What's the use case for debug CSA checkers in clang-tidy?

Thu, May 3, 9:54 AM
alexfh requested changes to D46187: [Analyzer] getRegisteredCheckers(): handle debug checkers too..
Thu, May 3, 9:54 AM
alexfh requested changes to D46159: [clang-tidy] Add a flag to enable alpha checkers.

I think the premise is a bit off the mark. It's not that these are not for the common user -- it's that they're simply not ready for users at all. Making it easier to expose does not seem like it serves users because those users expect exposed features to work.

Thu, May 3, 9:49 AM
alexfh requested changes to D33537: [clang-tidy] Exception Escape Checker.

It looks like you've missed some comments or uploaded a wrong patch.

Thu, May 3, 9:14 AM · Restricted Project
alexfh requested changes to D33844: [clang-tidy] terminating continue check.

There are still outstanding comments.

Thu, May 3, 9:10 AM · Restricted Project
alexfh committed rL331461: Added trailing periods..
Added trailing periods.
Thu, May 3, 9:08 AM
alexfh committed rCTE331461: Added trailing periods..
Added trailing periods.
Thu, May 3, 9:08 AM
alexfh requested changes to D40937: [clang-tidy] Infinite loop checker.
Thu, May 3, 9:08 AM · Restricted Project
alexfh committed rCTE331460: Add a trailing period in release notes..
Add a trailing period in release notes.
Thu, May 3, 9:03 AM
alexfh committed rL331460: Add a trailing period in release notes..
Add a trailing period in release notes.
Thu, May 3, 9:03 AM
alexfh requested changes to D46325: [clang-tidy] Define __clang_analyzer__ macro for clang-tidy for compatibility with clang static analyzer. 2nd try..
Thu, May 3, 8:54 AM · Restricted Project
alexfh requested changes to D45927: [clang-tidy] [modernize-use-auto] Correct way to calculate a type name length for multi-token types.
Thu, May 3, 8:53 AM · Restricted Project
alexfh added a comment to D46159: [clang-tidy] Add a flag to enable alpha checkers.

But still, could you explain the use case and why a local modification of clang-tidy is not an option?

Because I would like to direct users to try an alpha check on their local codebases without having to tell them to rebuild clang.

Okay, let's go with a hidden flag (something explicit enough, e.g. -allow-enabling-static-analyzer-alpha-checkers)

Thu, May 3, 8:19 AM
alexfh added a comment to D46159: [clang-tidy] Add a flag to enable alpha checkers.

But still, could you explain the use case and why a local modification of clang-tidy is not an option?

Because I would like to direct users to try an alpha check on their local codebases without having to tell them to rebuild clang.

Thu, May 3, 8:16 AM
alexfh added a comment to D46159: [clang-tidy] Add a flag to enable alpha checkers.

How about -enable-alpha-checks=yes-i-know-they-are-broken ?

Thu, May 3, 8:01 AM
alexfh requested changes to D46159: [clang-tidy] Add a flag to enable alpha checkers.
Thu, May 3, 7:54 AM
alexfh created D46393: Remove explicit cfg-temporary-dtors=true.
Thu, May 3, 7:50 AM
alexfh committed rL331456: [clang-tidy] Remove AnalyzeTemporaryDtors option..
[clang-tidy] Remove AnalyzeTemporaryDtors option.
Thu, May 3, 7:44 AM
alexfh committed rCTE331456: [clang-tidy] Remove AnalyzeTemporaryDtors option..
[clang-tidy] Remove AnalyzeTemporaryDtors option.
Thu, May 3, 7:44 AM
alexfh added a comment to D46159: [clang-tidy] Add a flag to enable alpha checkers.

As Devin says (and as we discussed this with Anna Zaks) alpha checkers are still in development, so we don't want to expose them to the users, even very curious ones. For those who want to help with development of the alpha checkers, there's always a possibility to change one boolean literal in clang-tidy sources and build their own clang-tidy binary. Or one can use clang -analyze. Are there any use cases not covered by these two options?

Thu, May 3, 6:33 AM

Wed, May 2

alexfh added inline comments to D45932: [clang-tidy][modernize-raw-string-literal] Don't replace upper ASCII with raw literals.
Wed, May 2, 3:43 AM · Restricted Project

Mon, Apr 30

alexfh added inline comments to D46293: [clang-tidy/google-runtime-int] Allow passing non-bitwidth types to printf()-style APIs.
Mon, Apr 30, 4:09 PM
alexfh accepted D46293: [clang-tidy/google-runtime-int] Allow passing non-bitwidth types to printf()-style APIs.

LG with one comment. Thank you for the fix!

Mon, Apr 30, 4:03 PM
alexfh added a comment to D45931: [ASTMatchers] Don't garble the profiling output when multiple TU's are processed.

From a user's perspective I'd probably prefer a different behavior of checks profiling with multiple translation units: per-file table after each file and an aggregate table at the end. An independent improvement could be to support TSV/CSV output and/or dumping to a file to spare the user from parsing the tables out of the stdout/stderr.

Mon, Apr 30, 11:42 AM
alexfh added a comment to D45927: [clang-tidy] [modernize-use-auto] Correct way to calculate a type name length for multi-token types.

I think, it's 13, if you choose to remove stars, and 17 otherwise. The difference is excessive spaces vs. required ones. Implementing proper logic may be involved, but we can simplify it to something like "count all non-space characters and a single space between words, but don't count spaces around punctuation":

Isn't it a business of clang-format to determine the number of spaces between lexemes? IMO the solutuion you've provided for GetTypeNameLength is enough. Considering punctuation here is overkill :-)

Mon, Apr 30, 11:32 AM · Restricted Project
alexfh added inline comments to D46233: [ASTMatchers] Overload isConstexpr for ifStmts.
Mon, Apr 30, 11:16 AM
alexfh committed rL331207: Regenerated AST Matchers doc..
Regenerated AST Matchers doc.
Mon, Apr 30, 11:15 AM
alexfh committed rC331207: Regenerated AST Matchers doc..
Regenerated AST Matchers doc.
Mon, Apr 30, 11:15 AM
alexfh requested changes to D46233: [ASTMatchers] Overload isConstexpr for ifStmts.
Mon, Apr 30, 10:44 AM
alexfh added a comment to D45927: [clang-tidy] [modernize-use-auto] Correct way to calculate a type name length for multi-token types.

Actually, just ignoring spaces may be not the best option.

Mon, Apr 30, 9:21 AM · Restricted Project
alexfh requested changes to D45927: [clang-tidy] [modernize-use-auto] Correct way to calculate a type name length for multi-token types.
Mon, Apr 30, 8:10 AM · Restricted Project

Thu, Apr 26

alexfh added inline comments to D46146: [analyzer] pr37152: Fix operator delete[] array-type-sub-expression handling..
Thu, Apr 26, 3:17 PM

Wed, Apr 25

alexfh added reviewers for D46027: [clang-tidy] Fix PR35824: rsmith, aaron.ballman.
Wed, Apr 25, 3:40 AM

Tue, Apr 24

alexfh accepted D46003: [clang-tidy] Fix PR35468.

LG with one comment.

Tue, Apr 24, 6:28 AM

Apr 22 2018

alexfh added a comment to D45718: [analyzer] Allow registering custom statically-linked analyzer checkers.

Actually, we do have unittests in tools/clang/unittest/Analysis.
@alexfh would you be able to write a unittest in there? It should be fairly easy following the structure of e.g. tools/clang/Analysis/CloneDetectionTest.cpp.
This would also help to ensure this entry point does not bit rot.

Apr 22 2018, 12:00 PM

Apr 21 2018

alexfh accepted D45776: [clang-tidy] Customize FileCheck prefix in check_clang-tidy.py.

LG

Apr 21 2018, 6:39 AM · Restricted Project

Apr 20 2018

alexfh accepted D45912: update test to use ivar in implementation instead of class extension.

LG

Apr 20 2018, 10:51 PM
alexfh added a comment to D45776: [clang-tidy] Customize FileCheck prefix in check_clang-tidy.py.

A few style-related comments.

Apr 20 2018, 4:53 PM · Restricted Project
alexfh accepted D45160: [clang-apply-replacements] Make clang-apply-replacements installable.

LG

Apr 20 2018, 4:36 PM · Restricted Project
alexfh accepted D45392: [clang-tidy] add new check to find out objc ivars which do not have prefix '_'.

LG. Thanks!

Apr 20 2018, 12:21 AM · Restricted Project

Apr 18 2018

alexfh requested changes to D45776: [clang-tidy] Customize FileCheck prefix in check_clang-tidy.py.
Apr 18 2018, 3:08 PM · Restricted Project
alexfh added a comment to D45776: [clang-tidy] Customize FileCheck prefix in check_clang-tidy.py.

Thank you for the contribution!
Please update the documentation accordingly (http://clang.llvm.org/extra/clang-tidy/#testing-checks).

Apr 18 2018, 3:07 PM · Restricted Project

Apr 17 2018

alexfh accepted D45697: [clang-tidy] Fix clang-tidy doesn't read .clangtidy configuration file..
Apr 17 2018, 6:40 AM
alexfh created D45718: [analyzer] Allow registering custom statically-linked analyzer checkers.
Apr 17 2018, 3:48 AM
alexfh requested changes to D45697: [clang-tidy] Fix clang-tidy doesn't read .clangtidy configuration file..

Thank you for investigating and fixing this!

Apr 17 2018, 3:23 AM

Apr 16 2018

alexfh removed a reviewer for D45662: OpenBSD add C++ runtime in a driver's standpoint: alexfh.
Apr 16 2018, 3:49 AM

Apr 12 2018

alexfh added a comment to D45392: [clang-tidy] add new check to find out objc ivars which do not have prefix '_'.

BTW, it looks like the http://clang.llvm.org/extra/clang-tidy/checks/objc-property-declaration.html check could also be replaced with readability-identifier-naming (not in this patch).

Apr 12 2018, 6:39 AM · Restricted Project
alexfh requested changes to D45392: [clang-tidy] add new check to find out objc ivars which do not have prefix '_'.
Apr 12 2018, 6:37 AM · Restricted Project
alexfh added inline comments to D45392: [clang-tidy] add new check to find out objc ivars which do not have prefix '_'.
Apr 12 2018, 4:09 AM · Restricted Project
alexfh added a comment to D44602: [clang-tidy] readability-function-size: add VariableThreshold param..

@alexfh i'm waiting for some comment from you. there was no comment added when you removed yourself as a reviewer,
so i *assume* the usual message of that time was meant - "removing from my review queue, re-add when others have accepted.".

Apr 12 2018, 3:45 AM · Restricted Project

Apr 11 2018

alexfh requested changes to D45392: [clang-tidy] add new check to find out objc ivars which do not have prefix '_'.

Thank you, that's much better!

Apr 11 2018, 6:37 AM · Restricted Project
alexfh added a comment to D45405: [clang-tidy] [modernize-use-auto] Add a threshold for minimal type name length to be replaced with 'auto'.

Roman, I see you've fixed them. Thanks a lot!
I did not face with these errors on MSVS'201 so had no chance to fix early.

Apr 11 2018, 6:26 AM · Restricted Project