aaron.ballman (Aaron Ballman)
User

Projects

User does not belong to any projects.

User Details

User Since
Mar 14 2013, 3:16 PM (247 w, 4 d)

Recent Activity

Today

aaron.ballman accepted D41080: Don't trigger -Wuser-defined-literals for system headers.

LGTM (wait for a day or so for @rsmith to respond in case he disagrees).

Mon, Dec 11, 1:40 PM
aaron.ballman added a comment to D41080: Don't trigger -Wuser-defined-literals for system headers.

This is missing a test case.

Mon, Dec 11, 11:19 AM
aaron.ballman added a comment to D41056: [clang-tidy] New check misc-uniqueptr-release-unused-retval.

May be bugprone is better module then misc?

Maybe. I can move it if all the reviewers think that it would be better suited there.

Yup, bugprone- should be a better category for this, IMO.

I wonder whether libc++ folks are interested in marking unique_ptr::release() with __attribute__ ((warn_unused_result)). A compiler warning (with -Werror) would be a better protection against this kind of a bug.

Mon, Dec 11, 8:48 AM · Restricted Project
aaron.ballman added a comment to D41064: Suppress -Wuser-defined-literals for <string> and <string_view>.

I think that it would be more appropriate to fix this in Clang rather than libc++. For instance, we don't want libstdc++ to have to silence our same diagnostic here.

Mon, Dec 11, 5:03 AM

Fri, Dec 8

aaron.ballman added a comment to D40854: [clang-tidy] WIP implement cppcoreguidelines check for mixed integer arithmetic.

The "enforcement" listed on the C++ Core Guidelines is very unhelpful, so it might be worth bringing it up as an issue on their bug tracker. ES.100 basically says "you know what we mean, wink wink" as enforcement and doesn't give any guidance as to what is safe or unsafe. It gives no exceptions, which I think is unlikely to be useful to most developers. For instance: void f(unsigned i) { if (i > 0) {} }, this is a mixed signed and unsigned comparison (literals are signed by default) -- should this check diagnose? How about unsigned int i = 12; i += 1;? ES.102 is equally as strange with "Flag unsigned literals (e.g. -2) used as container subscripts." That's a signed literal (2) with a negation operator -- do they mean "Flag container subscripts whose value is known to be negative", or something else?

Fri, Dec 8, 12:51 PM
aaron.ballman added inline comments to D40625: Harmonizing attribute GNU/C++ spellings.
Fri, Dec 8, 12:33 PM
aaron.ballman added inline comments to D40787: [clang-tidy] Replace the usage of std::uncaught_exception with std::uncaught_exceptions.
Fri, Dec 8, 12:21 PM · Restricted Project
aaron.ballman accepted D41016: [Sema] Fix crash in unused-lambda-capture warning for VLAs.

LGTM!

Fri, Dec 8, 12:20 PM

Thu, Dec 7

aaron.ballman added a reviewer for D40937: [clang-tidy] Infinite loop checker: dcoughlin.

I share the concerns that @Eugene.Zelenko brought up regarding this not being in the analyzer. This is a path sensitive problem that I'm not certain clang-tidy is the best home for. You cover many of the simple cases, but fail to cover cases like nested loops, unsigned integer wrapping, non-integral types, function calls, globals, etc. I'm not certain this check will generalize over time as part of clang-tidy in the way it would in the analyzer. I'd be curious to hear what @alexfh and @dcoughlin think, though.

Thu, Dec 7, 2:16 PM · Restricted Project
aaron.ballman requested changes to D40671: [clang-tidy] Support specific checks for NOLINT directive.
In D40671#947762, @xgsa wrote:

So can this patch be submitted? Should I do something to make it happen?

Thu, Dec 7, 2:08 PM · Restricted Project
aaron.ballman closed D40225: Add -std=c17 as a flag.

Commit in r320089.

Thu, Dec 7, 1:47 PM
aaron.ballman closed D39665: Support __has_c_attribute.

Commit in r320088.

Thu, Dec 7, 1:38 PM
aaron.ballman updated the diff for D40448: Add a printing policy for AST dumping.

Ping. Added more context to the patch.

Thu, Dec 7, 12:08 PM
aaron.ballman added a comment to D40225: Add -std=c17 as a flag.

Rebased on ToT and given more context.

Thu, Dec 7, 12:06 PM
aaron.ballman updated the diff for D40625: Harmonizing attribute GNU/C++ spellings.

Updated based on review feedback. This patch leaves off attributes with custom parsing, as well as analyzer_noreturn, so that we can handle them as special cases.

Thu, Dec 7, 12:05 PM
aaron.ballman added inline comments to D40625: Harmonizing attribute GNU/C++ spellings.
Thu, Dec 7, 12:01 PM

Wed, Dec 6

aaron.ballman accepted D40895: Ignore pointers to incomplete types when diagnosing misaligned addresses.

LGTM, thank you!

Wed, Dec 6, 8:43 AM
aaron.ballman added inline comments to D40813: [clang-tidy] Adding Fuchsia checker for virtual inheritance.
Wed, Dec 6, 8:28 AM · Restricted Project

Tue, Dec 5

aaron.ballman accepted D40829: [clang-tidy] adjust cppcoreguidelines-owning-memory documentation.

This is reasonable, though if the list starts getting longer, we should consider trying to fix the bug rather than enumerate them.

Tue, Dec 5, 8:04 AM
aaron.ballman added inline comments to D40813: [clang-tidy] Adding Fuchsia checker for virtual inheritance.
Tue, Dec 5, 7:54 AM · Restricted Project
aaron.ballman added inline comments to D40580: [clang-tidy] Adding Fuchsia checker for multiple inheritance.
Tue, Dec 5, 7:45 AM · Restricted Project

Mon, Dec 4

aaron.ballman added inline comments to D40625: Harmonizing attribute GNU/C++ spellings.
Mon, Dec 4, 1:40 PM
aaron.ballman updated the diff for D40225: Add -std=c17 as a flag.

Rebased on ToT and given more context.

Mon, Dec 4, 1:03 PM
aaron.ballman added inline comments to D40787: [clang-tidy] Replace the usage of std::uncaught_exception with std::uncaught_exceptions.
Mon, Dec 4, 12:23 PM · Restricted Project
aaron.ballman added inline comments to D40671: [clang-tidy] Support specific checks for NOLINT directive.
Mon, Dec 4, 10:12 AM · Restricted Project

Sun, Dec 3

aaron.ballman accepted D40671: [clang-tidy] Support specific checks for NOLINT directive.

Aside from a minor commenting nit, this LGTM. Thanks!

Sun, Dec 3, 11:13 AM · Restricted Project
aaron.ballman added a comment to D40225: Add -std=c17 as a flag.

Ping

Sun, Dec 3, 8:00 AM
aaron.ballman added a comment to D39665: Support __has_c_attribute.

Ping

Sun, Dec 3, 7:59 AM
aaron.ballman added a comment to D40737: [clang-tidy] WIP Resubmit hicpp-multiway-paths-covered without breaking test.

I would like to see the large test case added back in -- it was demonstrating a real problem with the code and it would be good to ensure we don't regress that behavior again in the future.

Sun, Dec 3, 7:59 AM
aaron.ballman added inline comments to D40671: [clang-tidy] Support specific checks for NOLINT directive.
Sun, Dec 3, 7:51 AM · Restricted Project
aaron.ballman added inline comments to D40580: [clang-tidy] Adding Fuchsia checker for multiple inheritance.
Sun, Dec 3, 7:30 AM · Restricted Project

Fri, Dec 1

aaron.ballman added inline comments to D40580: [clang-tidy] Adding Fuchsia checker for multiple inheritance.
Fri, Dec 1, 10:34 AM · Restricted Project
aaron.ballman added inline comments to D40580: [clang-tidy] Adding Fuchsia checker for multiple inheritance.
Fri, Dec 1, 10:09 AM · Restricted Project
aaron.ballman added a comment to D40671: [clang-tidy] Support specific checks for NOLINT directive.

This feature should probably be mentioned in the release notes.

Fri, Dec 1, 9:27 AM · Restricted Project
aaron.ballman added inline comments to D40625: Harmonizing attribute GNU/C++ spellings.
Fri, Dec 1, 9:03 AM
aaron.ballman closed D15406: Add warning for attribute-cleanup on function parameter..

I committed a different fix for this in r319555.

Fri, Dec 1, 8:54 AM
aaron.ballman added inline comments to D40671: [clang-tidy] Support specific checks for NOLINT directive.
Fri, Dec 1, 6:41 AM · Restricted Project
aaron.ballman added a comment to D38921: [analyzer] LoopUnrolling: update the matched assignment operators.

Aside from a request for another test, the matcher parts LGTM.

Fri, Dec 1, 5:31 AM

Thu, Nov 30

aaron.ballman added a comment to D40448: Add a printing policy for AST dumping.

Ping.

Thu, Nov 30, 9:40 AM
aaron.ballman updated subscribers of D40625: Harmonizing attribute GNU/C++ spellings.

Added @dcoughlin for opinions about the static analyzer, added @sbaranga and @jmolloy for questions about NEON.

Thu, Nov 30, 7:08 AM

Wed, Nov 29

aaron.ballman closed D40574: Bounds check argument_with_type_tag attribute..

Thanks for the approval Aaron. I do not have commit access, would you mind submitting this on my behalf?

Wed, Nov 29, 3:11 PM
aaron.ballman created D40625: Harmonizing attribute GNU/C++ spellings.
Wed, Nov 29, 3:07 PM
aaron.ballman accepted D40574: Bounds check argument_with_type_tag attribute..

Thanks, LGTM!

Wed, Nov 29, 1:23 PM
aaron.ballman closed D40611: Add has definition AST matcher.

Commit in r319360

Wed, Nov 29, 1:22 PM
aaron.ballman accepted D40611: Add has definition AST matcher.

Aside from a minor commenting nit, LGTM! Do you need me to commit this on your behalf? (If so, I can take care of the commenting fix.)

Wed, Nov 29, 12:49 PM
aaron.ballman added inline comments to D40580: [clang-tidy] Adding Fuchsia checker for multiple inheritance.
Wed, Nov 29, 8:49 AM · Restricted Project
aaron.ballman added inline comments to D40574: Bounds check argument_with_type_tag attribute..
Wed, Nov 29, 8:30 AM

Tue, Nov 28

aaron.ballman closed D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy.

I've commit in r319225.

Tue, Nov 28, 1:10 PM · Restricted Project
aaron.ballman added a comment to D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy.

Thanks for the nit fix. If you'd like me to commit this on your behalf, just let me know.

Tue, Nov 28, 10:01 AM · Restricted Project
aaron.ballman accepted D40505: [clang-tidy] Ignore ExprWithCleanups when looking for else-after-throw.

LGTM with a minor formatting nit.

Tue, Nov 28, 6:04 AM
aaron.ballman accepted D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy.

LGTM aside from one small nit with the test file missing a trailing newline.

Tue, Nov 28, 6:01 AM · Restricted Project

Mon, Nov 27

aaron.ballman closed D39863: [clang-tidy] Relax the way misc-move-const-arg treats trivially copyable types.

Thank you for the ping, this fell off my radar (sorry about the delay in committing it) -- I've commit in r319111.

Mon, Nov 27, 3:00 PM · Restricted Project
aaron.ballman accepted D40516: [clang-tidy] Correcting tests for ReplaceRandomShuffleCheck.

One small typo with one of the test changes, but otherwise LGTM, good catch!

Mon, Nov 27, 12:52 PM · Restricted Project
aaron.ballman requested changes to D39857: [AMDGPU] Late parsed / dependent arguments for AMDGPU kernel attributes.

Please add test cases for this new functionality.

Mon, Nov 27, 5:52 AM · Restricted Project
aaron.ballman accepted D39243: [clang-tidy] Misc redundant expressions checker updated for overloaded operators.

LGTM!

Mon, Nov 27, 5:45 AM
aaron.ballman accepted D40487: [clang-tidy] Move checks from misc- to performance-.

LGTM!

Mon, Nov 27, 4:56 AM

Sun, Nov 26

aaron.ballman updated the diff for D33563: Track whether a unary operation can overflow.

Rebased against ToT, ping.

Sun, Nov 26, 4:26 PM
aaron.ballman added a comment to D18768: Refactoring attribute subject diagnostics.

I had the chance to finalize this long-standing review by fixing up the remaining test cases and removing the unused enumerators from AttributeList.h. I've commit in r319002 (but cannot close the review because the "Accept" didn't happen).

Sun, Nov 26, 12:02 PM
aaron.ballman updated the diff for D15935: Improve diagnostics for literal conversion to Boolean.

Ping. Rebased on ToT.

Sun, Nov 26, 8:31 AM
aaron.ballman abandoned D33788: Return a canonical path from getClangResourcePath().
Sun, Nov 26, 6:30 AM
aaron.ballman resigned from D19201: [clang-tidy] misc-throw-with-noexcept.
Sun, Nov 26, 6:28 AM
aaron.ballman added a comment to D40225: Add -std=c17 as a flag.

Ping

Sun, Nov 26, 6:27 AM
aaron.ballman added a comment to D39665: Support __has_c_attribute.

Ping

Sun, Nov 26, 6:27 AM

Sat, Nov 25

aaron.ballman requested changes to D39913: [ObjC] warn about availability attributes missing from a method's declaration when they're specified for a method's definition .
Sat, Nov 25, 6:48 AM

Fri, Nov 24

aaron.ballman added a comment to D30610: [clang-tidy] Added options to cppcoreguidelines-special-member-functions check.

I have 1000s of warnings from this check.

A lot of them are about google tests:
warning: class 'Foo_Bar_Test' defines a copy constructor and a copy assignment operator but does not define a destructor [cppcoreguidelines-special-member-functions]
I'd like an option to suppress these.

It warns about classes that use boost::noncopyable:
warning: class 'Foo' defines a non-default destructor but does not define a copy constructor or a copy assignment operator [cppcoreguidelines-special-member-functions]
class Foo : boost::noncopyable
I think this is a bug in the check.

Fri, Nov 24, 3:32 PM
aaron.ballman created D40448: Add a printing policy for AST dumping.
Fri, Nov 24, 2:33 PM
aaron.ballman added a comment to D39722: [ASTImporter] Support TypeTraitExpr Importing.

Hello Aaron,

I remove the semicolon.

Is this type actually correct for C++?

Yes, it is.
clang generates the AST for declToImport struct like this.

|-CXXRecordDecl 0x8b19fe0 <col:22, col:29> col:29 implicit struct declToImport
`-CXXMethodDecl 0x8b1a0d0 <line:2:3, col:27> col:8 m 'void (void)'
  `-CompoundStmt 0x8b1a1e8 <col:12, col:27>
    `-TypeTraitExpr 0x8b1a1c8 <col:14, col:24> '_Bool'

Hmm, that looks like a bug to me; but not one that should impact this review.

Fri, Nov 24, 10:54 AM
aaron.ballman accepted D39722: [ASTImporter] Support TypeTraitExpr Importing.

Hello Aaron,

I remove the semicolon.

Is this type actually correct for C++?

Yes, it is.
clang generates the AST for declToImport struct like this.

|-CXXRecordDecl 0x8b19fe0 <col:22, col:29> col:29 implicit struct declToImport
`-CXXMethodDecl 0x8b1a0d0 <line:2:3, col:27> col:8 m 'void (void)'
  `-CompoundStmt 0x8b1a1e8 <col:12, col:27>
    `-TypeTraitExpr 0x8b1a1c8 <col:14, col:24> '_Bool'
Fri, Nov 24, 10:42 AM
aaron.ballman added inline comments to D39722: [ASTImporter] Support TypeTraitExpr Importing.
Fri, Nov 24, 10:15 AM
aaron.ballman added a comment to D30610: [clang-tidy] Added options to cppcoreguidelines-special-member-functions check.

I'd like an AllowDeletedCopyFunctions option that allows move and destructor functions to be missing when copying is disabled.

struct A {
  A(const A&) = delete;
  A& operator=(const A&) = delete;
}

Doesn't AllowMissingMoveFunctions do almost that? If not, it should -- that code produces a class that does not declare a move constructor or move assignment operator per [class.copy]p8 and [class.copy.assign]p4, so that would be a "missing move function". Granted, that doesn't handle the dtor case, but I think an AllowMissingDestructor option might be overkill -- the destructor isn't missing, it's implicitly declared as defaulted in that case, but if the C++ Core Guidelines folks want it spelled out explicitly then, it might be worth the option. Have they weighed in on your exception?

The check is about providing a consistent set of special member functions.

Fri, Nov 24, 9:59 AM
aaron.ballman added a comment to D30610: [clang-tidy] Added options to cppcoreguidelines-special-member-functions check.

I'd like an AllowDeletedCopyFunctions option that allows move and destructor functions to be missing when copying is disabled.

struct A {
  A(const A&) = delete;
  A& operator=(const A&) = delete;
}
Fri, Nov 24, 7:19 AM

Thu, Nov 23

aaron.ballman added inline comments to D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy.
Thu, Nov 23, 8:16 AM · Restricted Project
aaron.ballman added a comment to D39722: [ASTImporter] Support TypeTraitExpr Importing.

Hello Takafumi,

This is almost OK to me but there is an inline comment we need to resolve in order to avoid Windows buildbot failures.
In addition, as Gabor pointed, when we add a new matcher, we need to update matcher documentation as well. To update the docs, you should perform just three steps.

  1. Rebase your patch onto latest master
  2. Launch script docs/tools/dump_ast_matchers.py
  3. Add changes made by this script to your commit.

I'm a bit confused -- I don't see any ASTMatcher changes as part of this patch aside from what's in the test file. Are you suggesting the matcher there should be hoisted as a separate patch?

In a previous version the matcher was public. I think it either should be public with documentation or it can remain in the test file and no doc update required. I am fine with both solution.

Thu, Nov 23, 8:11 AM
aaron.ballman added a comment to D39722: [ASTImporter] Support TypeTraitExpr Importing.

Hello Takafumi,

This is almost OK to me but there is an inline comment we need to resolve in order to avoid Windows buildbot failures.
In addition, as Gabor pointed, when we add a new matcher, we need to update matcher documentation as well. To update the docs, you should perform just three steps.

  1. Rebase your patch onto latest master
  2. Launch script docs/tools/dump_ast_matchers.py
  3. Add changes made by this script to your commit.
Thu, Nov 23, 7:55 AM

Wed, Nov 22

aaron.ballman added inline comments to D39243: [clang-tidy] Misc redundant expressions checker updated for overloaded operators.
Wed, Nov 22, 8:26 AM

Tue, Nov 21

aaron.ballman accepted D40242: Do not perform the analysis based warning if all warnings are ignored.

LGTM!

Tue, Nov 21, 12:02 PM
aaron.ballman added inline comments to D39243: [clang-tidy] Misc redundant expressions checker updated for overloaded operators.
Tue, Nov 21, 11:58 AM
aaron.ballman added a comment to D39285: [clang-tidy] Misc redundant expression checker updated for ineffective bitwise operator expressions.

This patch needs to be rebased off trunk before it can be applied cleanly.

https://reviews.llvm.org/D39243 is set as a dependency, so I suspect this can be applied cleanly once https://reviews.llvm.org/D39243 landed.

Tue, Nov 21, 11:43 AM
aaron.ballman added inline comments to D39722: [ASTImporter] Support TypeTraitExpr Importing.
Tue, Nov 21, 11:43 AM
aaron.ballman added a comment to D39285: [clang-tidy] Misc redundant expression checker updated for ineffective bitwise operator expressions.

This patch needs to be rebased off trunk before it can be applied cleanly.

Tue, Nov 21, 11:33 AM
aaron.ballman closed D40267: WG14 DR496.

Commit in r318796.

Tue, Nov 21, 11:26 AM
aaron.ballman closed D40261: Add default argument AST matcher.

I've commit in r318794, thank you!

Tue, Nov 21, 11:23 AM

Mon, Nov 20

aaron.ballman added a comment to D40242: Do not perform the analysis based warning if all warnings are ignored.

You should add a test case that demonstrates code which would otherwise trigger an analysis-based warning but doesn't due to disabling all warnings.

No warnings are triggered. Because the diagnostic engine ignores them.
This just optimize the code so no analysis are preformed when no warning can be triggered.

Mon, Nov 20, 11:02 PM
aaron.ballman added a comment to D40108: [clang-tidy] Adding Fuchsia checkers to clang-tidy.

Did you decide on fuchsia instead of zircon for the module name, or is that decision still up in the air?

Mon, Nov 20, 10:34 PM · Restricted Project
aaron.ballman added inline comments to D39454: [clang-tidy] Misc redundant expressions checker updated for confused operators.
Mon, Nov 20, 10:17 PM
aaron.ballman added a comment to D40261: Add default argument AST matcher.

I did find a few more spurious semicolons with the testing code as well.

Mon, Nov 20, 5:54 PM
aaron.ballman created D40267: WG14 DR496.
Mon, Nov 20, 1:39 PM
aaron.ballman accepted D40261: Add default argument AST matcher.

Aside from a minor nit with the comments leading to the public docs, LGTM!

Mon, Nov 20, 1:25 PM
aaron.ballman added a comment to D40242: Do not perform the analysis based warning if all warnings are ignored.

You should add a test case that demonstrates code which would otherwise trigger an analysis-based warning but doesn't due to disabling all warnings.

Mon, Nov 20, 12:56 PM
aaron.ballman added a comment to D40261: Add default argument AST matcher.

You should also update lib\ASTMatchers\Dynamic\Registry.cpp to have the new matcher (be sure to keep the new matcher alphabetized as well).

Mon, Nov 20, 12:40 PM

Sun, Nov 19

aaron.ballman created D40225: Add -std=c17 as a flag.
Sun, Nov 19, 9:19 AM

Sat, Nov 18

aaron.ballman added a comment to D39863: [clang-tidy] Relax the way misc-move-const-arg treats trivially copyable types.
In D39863#929572, @oleg wrote:

I've just rebased the patch over the top of LLVM/Clang/tools-extra master branches and tests pass: make check-clang-tools.

Could one of the reviewers commit this please?

Sat, Nov 18, 5:17 PM · Restricted Project
aaron.ballman added a comment to D39937: [Sema] Improve diagnostics for const- and ref-qualified member functions.

Thanks, will do. Is there an automated system that can run all the tests before I merge rather than waiting for a potential build failure after the fact?

Sat, Nov 18, 3:06 PM
aaron.ballman accepted D39937: [Sema] Improve diagnostics for const- and ref-qualified member functions.

This LGTM, but you should wait for a few days before committing in case @rsmith has comments.

Sat, Nov 18, 2:58 PM
aaron.ballman added inline comments to D39937: [Sema] Improve diagnostics for const- and ref-qualified member functions.
Sat, Nov 18, 2:30 PM
aaron.ballman added a comment to D39863: [clang-tidy] Relax the way misc-move-const-arg treats trivially copyable types.
In D39863#929429, @oleg wrote:

Thank you for the review, Aaron!

What's the lifecycle of these patches? Could you commit it on my behalf please?

Sat, Nov 18, 12:55 PM · Restricted Project
aaron.ballman added inline comments to D39454: [clang-tidy] Misc redundant expressions checker updated for confused operators.
Sat, Nov 18, 11:59 AM
aaron.ballman accepted D39285: [clang-tidy] Misc redundant expression checker updated for ineffective bitwise operator expressions.

LGTM!

Sat, Nov 18, 11:52 AM
aaron.ballman added inline comments to D39937: [Sema] Improve diagnostics for const- and ref-qualified member functions.
Sat, Nov 18, 11:48 AM
aaron.ballman accepted D39863: [clang-tidy] Relax the way misc-move-const-arg treats trivially copyable types.

LGTM aside from some small nits.

Sat, Nov 18, 11:45 AM · Restricted Project