aaron.ballman (Aaron Ballman)
User

Projects

User does not belong to any projects.

User Details

User Since
Mar 14 2013, 3:16 PM (231 w, 5 d)

Recent Activity

Wed, Aug 16

aaron.ballman added inline comments to D36672: [clang-tidy] readability-non-const-parameter: fixit on all function declarations.
Wed, Aug 16, 6:54 AM · Restricted Project
aaron.ballman added inline comments to D36354: [clang-tidy] Implement type-based check for `gsl::owner`.
Wed, Aug 16, 6:48 AM
aaron.ballman added inline comments to D36586: [clang-tidy] hicpp bitwise operations on signed integers.
Wed, Aug 16, 6:42 AM

Mon, Aug 14

aaron.ballman added a comment to rL310727: Add hicpp-exception-baseclass to the HIC++ module..

@JonasToth
@aaron.ballman

Ok, i missed the diff revision somehow, https://reviews.llvm.org/D36583

  • The diagnostic for templates are useless, e.g. in a such case: (greatly simplified, but hopefully gets the idea across) ` template <typename T> [[noreturn]] void attribute((noreturn)) ThrowException() { throw T; }

    class MyException : public std::runtime_error { ... };

    #define ThrowExceptionHelper(CLASS) ThrowException<CLASS>()

    void fun() { ThrowExceptionHelper(MyException); } ` All the output is: ` ...: error: throwing an exception whose type is not derived from 'std::exception' [hicpp-exception-baseclass,-warnings-as-errors] throw T; ^ ...: note: type defined here template <typename T> ^ `
  • There is no test with templated code

That should be rectified -- good catch!

  • Is exception supposed to be *only* *directly* inherited from std::exception, or can it inherit from std::runtime_error? In either case, tests should answer that.
  • What about inheriting from a exception which is *directly* inherited from std::exception? In either case, tests should answer that.

I take it to mean any level of inheritance is acceptable, including multiple inheritance.

Well, right now it complains on MyException, which is derived from std::runtime_error...
This really really needs more tests.

Mon, Aug 14, 5:35 AM
aaron.ballman added a comment to rL310727: Add hicpp-exception-baseclass to the HIC++ module..

@JonasToth
@aaron.ballman

Ok, i missed the diff revision somehow, https://reviews.llvm.org/D36583

  • The diagnostic for templates are useless, e.g. in a such case: (greatly simplified, but hopefully gets the idea across) ` template <typename T> [[noreturn]] void attribute((noreturn)) ThrowException() { throw T; }

    class MyException : public std::runtime_error { ... };

    #define ThrowExceptionHelper(CLASS) ThrowException<CLASS>()

    void fun() { ThrowExceptionHelper(MyException); } ` All the output is: ` ...: error: throwing an exception whose type is not derived from 'std::exception' [hicpp-exception-baseclass,-warnings-as-errors] throw T; ^ ...: note: type defined here template <typename T> ^ `
  • There is no test with templated code
Mon, Aug 14, 5:28 AM

Fri, Aug 11

aaron.ballman added inline comments to D36586: [clang-tidy] hicpp bitwise operations on signed integers.
Fri, Aug 11, 12:54 PM
aaron.ballman added inline comments to D36354: [clang-tidy] Implement type-based check for `gsl::owner`.
Fri, Aug 11, 11:46 AM
aaron.ballman closed D36583: [clang-tidy] add checker for hicpp to ensure all exceptions are derived from std::exception.

I've commit in r310727.

Fri, Aug 11, 9:32 AM
aaron.ballman added inline comments to D36586: [clang-tidy] hicpp bitwise operations on signed integers.
Fri, Aug 11, 9:21 AM
aaron.ballman updated subscribers of D36583: [clang-tidy] add checker for hicpp to ensure all exceptions are derived from std::exception.

Still got no commit access :(, but i asked already ;)

Could you please apply this patch?

Fri, Aug 11, 8:56 AM
aaron.ballman accepted D36583: [clang-tidy] add checker for hicpp to ensure all exceptions are derived from std::exception.

LGTM!

Fri, Aug 11, 6:59 AM
aaron.ballman added inline comments to D36586: [clang-tidy] hicpp bitwise operations on signed integers.
Fri, Aug 11, 6:23 AM
aaron.ballman added inline comments to D36354: [clang-tidy] Implement type-based check for `gsl::owner`.
Fri, Aug 11, 5:46 AM
aaron.ballman added inline comments to D36583: [clang-tidy] add checker for hicpp to ensure all exceptions are derived from std::exception.
Fri, Aug 11, 5:29 AM
aaron.ballman closed D36577: [clang-tidy] add an alias hicpp-braces-around-statements.

I've commit as r310707.

Fri, Aug 11, 5:15 AM · Restricted Project

Wed, Aug 9

aaron.ballman accepted D36526: [Sema] Assign new flag -Wenum-compare-switch to switch-related parts of -Wenum-compare.

LGTM

Wed, Aug 9, 1:08 PM
aaron.ballman accepted D36473: Fix broken getAttributeSpellingListIndex for pragma attributes.

LGTM, good catch!

Wed, Aug 9, 6:32 AM

Tue, Aug 8

aaron.ballman accepted D36208: [mips] Enable `long_call/short_call` attributes on MIPS64.

LGTM, thanks!

Tue, Aug 8, 12:48 PM · Restricted Project
aaron.ballman closed D36411: Restore previous structure ABI for bitfields with 'packed' attribute for PS4 targets.

I've commit in r310388

Tue, Aug 8, 11:08 AM
aaron.ballman accepted D36407: [Sema] Extend -Wenum-compare to handle mixed enum comparisons in switch statements.

Aside from a minor naming nit, LGTM!

Tue, Aug 8, 11:03 AM
aaron.ballman accepted D36411: Restore previous structure ABI for bitfields with 'packed' attribute for PS4 targets.

LGTM!

Tue, Aug 8, 8:21 AM
aaron.ballman accepted D35937: [clang-tidy] Add new readability non-idiomatic static access.

LGTM!

Tue, Aug 8, 8:16 AM · Restricted Project
aaron.ballman added a comment to D36407: [Sema] Extend -Wenum-compare to handle mixed enum comparisons in switch statements.

Can you generate the updated patch with more context (https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface)?

Tue, Aug 8, 8:07 AM
aaron.ballman accepted D36355: [clang-tidy] add forwarders in the aliased checks from hicpp module.

i dont have commit rights. i plan to now continuesly work on clang tools, is there a way to get these rights?

Tue, Aug 8, 7:54 AM
aaron.ballman added inline comments to D36354: [clang-tidy] Implement type-based check for `gsl::owner`.
Tue, Aug 8, 7:51 AM
aaron.ballman added inline comments to D35755: [Solaris] gcc toolchain handling revamp.
Tue, Aug 8, 7:46 AM
aaron.ballman added a comment to D35755: [Solaris] gcc toolchain handling revamp.

Aside from a coding style nit and the unanswered question that hopefully @tstellar can help answer, this LGTM. I'll wait to accept until we figure out the answer for Linux, however.

Tue, Aug 8, 6:22 AM
aaron.ballman added a comment to D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker.

As for the the loss of precision problem, in the special case of char the size of char is known. However does the analysis have the necessary information in this stage to know the size of an int for example? I found bit-width specifying information in the llvm::Type class which is used in the code generation phase. It could be done by checking on a per type basis, but then again, it could possibly lead to false positives. Correct me if I am wrong.

Tue, Aug 8, 6:14 AM
aaron.ballman added inline comments to D36208: [mips] Enable `long_call/short_call` attributes on MIPS64.
Tue, Aug 8, 5:44 AM · Restricted Project

Wed, Aug 2

aaron.ballman added inline comments to D35755: [Solaris] gcc toolchain handling revamp.
Wed, Aug 2, 10:33 AM
aaron.ballman added a comment to D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker.

Even though it is not undefined behavior in C, it can still cause surprising behavior for the users. I think maybe putting it into the optin package instead of cplusplus is better. What do you think?

Wed, Aug 2, 10:12 AM
aaron.ballman added inline comments to D35937: [clang-tidy] Add new readability non-idiomatic static access.
Wed, Aug 2, 10:04 AM · Restricted Project
aaron.ballman accepted D33676: Place implictly declared functions at block scope.

This looks reasonable to me, once it's been formatted. Let's give @rsmith a few days to comment before committing, though.

Wed, Aug 2, 9:24 AM
aaron.ballman added inline comments to D33676: Place implictly declared functions at block scope.
Wed, Aug 2, 7:23 AM
aaron.ballman added a comment to D36208: [mips] Enable `long_call/short_call` attributes on MIPS64.

@aaron.ballman I missed your first comments when I'd submitted mine.

I think for the interrupt attribute, it should be an error. Currently it's an implementation detail that it errors out in the backend but in principal it can be supported (I haven't gotten around to addressing it.)

For the interrupt attribute, I think it should behave the same as the other target-specific interrupt attributes unless there's a very good reason to be inconsistent.

My primary thought there was that the backend will error out with "LLVM ERROR: "interrupt" attribute is only supported for the O32 ABI on MIPS32R2+ at the present time." if the attribute is seen anywhere by the backend for MIPS64.

Wed, Aug 2, 6:52 AM · Restricted Project
aaron.ballman added a comment to D36208: [mips] Enable `long_call/short_call` attributes on MIPS64.

I think for the interrupt attribute, it should be an error. Currently it's an implementation detail that it errors out in the backend but in principal it can be supported (I haven't gotten around to addressing it.)

Wed, Aug 2, 5:39 AM · Restricted Project
aaron.ballman added a comment to D36208: [mips] Enable `long_call/short_call` attributes on MIPS64.

Currently there is no support in the backend for the interrupt attribute for mips64 / using N32 & N64 abis, it will give a fatal error. Previously the backend lacked support for the static relocation model which is an expected requirement for interrupt handlers.

microMIPS32r(3|6) is only supported for the O32 ABI, as microMIPS64R6 is deprecated and going to be removed. There is no support for the published microMIPS64R3 ISA.

I see. What is the better solution to handle unsupported attributes: a) silently ignore them; b) show a warning; c) show an error?

Wed, Aug 2, 4:34 AM · Restricted Project

Fri, Jul 28

aaron.ballman added inline comments to D35937: [clang-tidy] Add new readability non-idiomatic static access.
Fri, Jul 28, 5:10 AM · Restricted Project

Thu, Jul 27

aaron.ballman added a comment to D26350: Keep invalid Switch in the AST.

This looks reasonable to me, but you should wait for @rsmith to sign off before committing.

Thu, Jul 27, 7:54 AM
aaron.ballman added inline comments to D35937: [clang-tidy] Add new readability non-idiomatic static access.
Thu, Jul 27, 7:41 AM · Restricted Project

Tue, Jul 25

aaron.ballman added a comment to D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker.

Aaron, could you comment on the applicability of this check to C? Thanks in advance.

Tue, Jul 25, 6:44 AM
aaron.ballman added inline comments to D33829: [clang-tidy] avoid reserved names check.
Tue, Jul 25, 6:06 AM · Restricted Project
aaron.ballman added a comment to D35796: [analyzer] Misused polymorphic object checker.

How does this check differ from the -Wdelete-non-virtual-dtor warning class that comes out of the frontend?

Tue, Jul 25, 5:29 AM

Jul 20 2017

aaron.ballman accepted D35652: [clang] Fix handling of "%zd" format specifier in scanf.

LGTM with a small testing nit.

Jul 20 2017, 7:43 AM

Jul 18 2017

aaron.ballman accepted D35519: Add SEMA checking to attribute 'target'.

A few small nits, but otherwise LGTM

Jul 18 2017, 8:28 AM
aaron.ballman accepted D35484: Add a warning for missing '#pragma pack (pop)' and suspicious '#pragma pack' uses when including files.

LGTM!

Jul 18 2017, 7:02 AM
aaron.ballman added inline comments to D35520: [Sema] Improve diagnostic message for unavailable c++17 aligned allocation functions .
Jul 18 2017, 6:05 AM
aaron.ballman added inline comments to D35484: Add a warning for missing '#pragma pack (pop)' and suspicious '#pragma pack' uses when including files.
Jul 18 2017, 5:47 AM
aaron.ballman added inline comments to D35519: Add SEMA checking to attribute 'target'.
Jul 18 2017, 5:14 AM

Jul 15 2017

aaron.ballman accepted D35454: [c++2a] Add option -std=c++2a to enable support for potential/transitional C++2a features.

LGTM with a small commenting nit.

Jul 15 2017, 2:22 PM · Restricted Project
aaron.ballman added a comment to D35454: [c++2a] Add option -std=c++2a to enable support for potential/transitional C++2a features.

Has GCC picked the same name for their language standard? I want to make sure we're consistent there (and think this is the correct name).

Jul 15 2017, 12:24 PM · Restricted Project

Jul 14 2017

aaron.ballman added a comment to D26350: Keep invalid Switch in the AST.

The problem i'm trying to solve is precisely to keep as much as possible of the valid AST in the main AST, despite errors.
I've already done some work with r249982, r272962 and more, and there is still a lot to do. But the goal is to keep as much as possible of it.

Jul 14 2017, 4:06 PM
aaron.ballman accepted D35427: [clang] Fix handling of "%zd" format specifier.

LGTM!

Jul 14 2017, 2:23 PM
aaron.ballman added a reviewer for D35427: [clang] Fix handling of "%zd" format specifier: aaron.ballman.
Jul 14 2017, 11:23 AM
aaron.ballman closed D35337: [Solaris] gcc runtime dropped support for .ctors, switch to .init_array.

Committed in r308038.

Jul 14 2017, 10:50 AM
aaron.ballman added a comment to D26350: Keep invalid Switch in the AST.

You've explained how you are accomplishing this but not why. I don't think Clang typically keeps erroneous AST nodes in the tree. What kind of problem is this intended to solve?

Jul 14 2017, 5:30 AM
aaron.ballman edited reviewers for D26350: Keep invalid Switch in the AST, added: aaron.ballman; removed: cfe-commits.
Jul 14 2017, 5:28 AM

Jul 13 2017

aaron.ballman edited reviewers for D35337: [Solaris] gcc runtime dropped support for .ctors, switch to .init_array, added: dlj; removed: cfe-commits.
Jul 13 2017, 8:09 AM
aaron.ballman added a comment to D35337: [Solaris] gcc runtime dropped support for .ctors, switch to .init_array.

This looks reasonable to me, but I am not overly familiar with this part of the toolchain. You should wait for another LG before committing.

Jul 13 2017, 6:11 AM

Jul 12 2017

aaron.ballman accepted D35257: [clang-tidy] Add new modernize use unary assert check.

LGTM!

Jul 12 2017, 6:29 AM · Restricted Project
aaron.ballman added inline comments to D35257: [clang-tidy] Add new modernize use unary assert check.
Jul 12 2017, 5:33 AM · Restricted Project

Jul 11 2017

aaron.ballman added inline comments to D35257: [clang-tidy] Add new modernize use unary assert check.
Jul 11 2017, 8:20 AM · Restricted Project
aaron.ballman added inline comments to D35257: [clang-tidy] Add new modernize use unary assert check.
Jul 11 2017, 7:18 AM · Restricted Project
aaron.ballman accepted D35196: [ASTMatchers][NFC] integerLiteral(): Mention negative integers in documentation..

LGTM, thank you!

Jul 11 2017, 5:39 AM

Jul 10 2017

aaron.ballman added inline comments to D35196: [ASTMatchers][NFC] integerLiteral(): Mention negative integers in documentation..
Jul 10 2017, 10:56 AM
aaron.ballman added inline comments to D35196: [ASTMatchers][NFC] integerLiteral(): Mention negative integers in documentation..
Jul 10 2017, 9:10 AM

Jul 5 2017

aaron.ballman added inline comments to D33470: [clang-tidy] Add misc-default-numerics.
Jul 5 2017, 6:38 AM · Restricted Project

Jul 3 2017

aaron.ballman accepted D34932: [clang-tidy] Resolve cppcoreguidelines-pro-type-member-init false positive.

LGTM!

Jul 3 2017, 7:58 AM · Restricted Project

Jun 30 2017

aaron.ballman added inline comments to D34671: This is to address more command from Richard Smith for my change of https://reviews.llvm.org/D33333.
Jun 30 2017, 3:08 PM

Jun 28 2017

aaron.ballman added inline comments to D34449: [clang-tidy] Enable inline variable definitions in headers. .
Jun 28 2017, 12:31 PM · Restricted Project
aaron.ballman added inline comments to D34449: [clang-tidy] Enable inline variable definitions in headers. .
Jun 28 2017, 6:09 AM · Restricted Project

Jun 26 2017

aaron.ballman added a comment to D33470: [clang-tidy] Add misc-default-numerics.

Once you fix the typo in the check, can you run it over some large C++ code bases to see if it finds any results?

I tried it on LLVM code base (after fixing bug with the numeric_limits name) and it didn't find anything suspisious.
Unfortunatelly I don't have enough time to try it on different codebases, but I am weiling to fix any bug with this check if it would happen in the future.
The release 5.0 is near, so I would like to push it upstream. Does it sound good to you?

Jun 26 2017, 6:44 AM · Restricted Project
aaron.ballman accepted D34091: Support for querying the exception specification type through libclang.

LGTM!

Jun 26 2017, 6:05 AM

Jun 22 2017

aaron.ballman accepted D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier.

Aside from a few small nits with the test cases, this LGTM! You should hold off on committing for a day or two in case Richard or others have comments on the patch.

Jun 22 2017, 5:25 AM

Jun 21 2017

aaron.ballman added inline comments to D34449: [clang-tidy] Enable inline variable definitions in headers. .
Jun 21 2017, 7:35 AM · Restricted Project

Jun 16 2017

aaron.ballman added inline comments to D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier.
Jun 16 2017, 1:59 PM
aaron.ballman added inline comments to D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier.
Jun 16 2017, 1:46 PM
aaron.ballman added a comment to D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier.

A few more nits, but this feels like it's getting close to ready (at least, to me).

Jun 16 2017, 1:06 PM
aaron.ballman added a comment to D32332: Add support for transparent overloadable functions in clang.

This is generally looking good to me, with a few small nits. @rsmith, do you have thought on this?

Jun 16 2017, 6:55 AM
aaron.ballman added a comment to D33563: Track whether a unary operation can overflow.

Ping

Jun 16 2017, 6:27 AM
aaron.ballman added inline comments to D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier.
Jun 16 2017, 6:15 AM
aaron.ballman added inline comments to D33722: [clang-tidy] Add checker for undelegated copy of base classes.
Jun 16 2017, 5:26 AM · Restricted Project

Jun 15 2017

aaron.ballman added inline comments to D34091: Support for querying the exception specification type through libclang.
Jun 15 2017, 2:27 PM

Jun 14 2017

aaron.ballman closed D34179: [Frontend] PR32318 Handle -ast-dump-all properly..

Committed in r305432. You should consider reaching out to Chris Lattner for svn access!

Jun 14 2017, 5:01 PM
aaron.ballman accepted D34179: [Frontend] PR32318 Handle -ast-dump-all properly..

LGTM!

Jun 14 2017, 4:39 PM
aaron.ballman added a comment to D33904: Add a __has_attribute_enhancement macro to clang.

Why not just use __has_feature(overloadable_unmarked) or similar?

My impression was that __has_feature was was for larger features than tweaks to attributes. If this would be an appropriate use of __has_feature, though, I'm happy to keep things simple.

I'll update the other review with __has_feature. If it goes in with that, I'll abandon this.

Jun 14 2017, 4:38 PM
aaron.ballman added inline comments to D34091: Support for querying the exception specification type through libclang.
Jun 14 2017, 4:36 PM
aaron.ballman added a comment to rL305025: [ASTMatchers] temporary disable tests with floating suffix.

It turns out that these tests are failing because:

  • 1.2f == 1.2 is false, apparently 1.2 cannot be exactly represented as single-precision float. Without modifying the grammar, one can use floatLiteral(equals(1.2000000476837158203125)) to match 1.2f.
Jun 14 2017, 4:29 PM
aaron.ballman requested changes to D33722: [clang-tidy] Add checker for undelegated copy of base classes.
Jun 14 2017, 4:25 PM · Restricted Project
aaron.ballman added inline comments to D33841: [clang-tidy] redundant keyword check.
Jun 14 2017, 4:08 PM · Restricted Project

Jun 13 2017

aaron.ballman added a reviewer for D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier: rsmith.

Adding Richard to the review for some wider perspective than just mine on the overall design.

Jun 13 2017, 2:56 PM

Jun 8 2017

aaron.ballman accepted D33094: [ASTMatchers] Add clang-query support for equals matcher.

LGTM!

Jun 8 2017, 2:04 PM
aaron.ballman accepted D33135: [ASTMatchers] Add support for floatLiterals.

LGTM!

Jun 8 2017, 2:01 PM
aaron.ballman accepted D33735: [DebugInfo] Add ThisOrSelf attribute for emission of FlagObjectPointer..

Aside from one minor nit, LGTM!

Jun 8 2017, 1:56 PM

Jun 7 2017

aaron.ballman accepted D34002: [clang-tidy] When" -fno-exceptions is used", this warning is better to be suppressed..

Aside from one minor nit, LGTM

Jun 7 2017, 11:47 AM · Restricted Project
aaron.ballman added inline comments to D33735: [DebugInfo] Add ThisOrSelf attribute for emission of FlagObjectPointer..
Jun 7 2017, 7:55 AM
aaron.ballman added inline comments to D33735: [DebugInfo] Add ThisOrSelf attribute for emission of FlagObjectPointer..
Jun 7 2017, 7:43 AM
aaron.ballman accepted D33917: [clang-tidy] When" -fno-exceptions is used", this warning is better to be suppressed..

Aside from a minor suggestion, LGTM!

Jun 7 2017, 6:38 AM · Restricted Project

Jun 5 2017

aaron.ballman added inline comments to D33829: [clang-tidy] avoid reserved names check.
Jun 5 2017, 1:44 PM · Restricted Project
aaron.ballman added a comment to D33825: [clang-tidy] signal handler must be plain old function check.

This check is an interesting one. The rules around what is signal safe are changing for C++17 to be a bit more lenient than what the rules are for C++14. CERT's rule is written against C++14, and so the current behavior matches the rule wording. However, the *intent* of the rule is to ensure that only signal-safe functionality is used from a signal handler, and so from that perspective, I can imagine a user compiling for C++17 to want the relaxed rules to still comply with CERT's wording. What do you think?

Jun 5 2017, 6:11 AM · Restricted Project