Page MenuHomePhabricator

aaron.ballman (Aaron Ballman)
User

Projects

User does not belong to any projects.

User Details

User Since
Mar 14 2013, 3:16 PM (398 w, 1 d)

Recent Activity

Today

aaron.ballman added a comment to D90399: [clang-tidy] non-portable-integer-constant check.

Have you run this check over any large code bases to see just how often it triggers and whether the diagnostics are false positives or true positives? I have a sneaking suspicion that this will yield a *ton* of false positives and probably not a whole lot of true positives. For starters, the issue isn't really a bugprone one -- the recommendation is about the behavior when compiling for different architectures or with different compilers, so this is more of a portability issue. It may make sense to move this into the portability module instead. However, the other issue is: a lot of CERT recommendations are recommendations rather than rules because it's impossible to determine programmer intent from the source code. If the user is intentionally using these literals, how should they silence this diagnostic?

Fri, Oct 30, 11:38 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D83717: [clang-tidy] Add check fo SEI CERT item ENV32-C.

One of the concerns I have with this not being a cfg-only check is that most of the bad situations are not going to be caught by the clang-tidy version of the check.

...

Have you run this check over any large code bases to see if it currently catches any true positive diagnostics?

I have tried llvm, tmux, curl and tried codesearch.com to look for other sources containing atexit, but no clang-tidy results were found for this check (this is partly because it is hard to manually make the project results buildable). So it is hard to see whether this flow-sensitive approach would result in many false positives.

Fri, Oct 30, 11:01 AM · Restricted Project, Restricted Project, Restricted Project
aaron.ballman accepted D89899: [CodeGen] Implement [[likely]] and [[unlikely]] for the iteration statements.

LGTM aside from a request about the note. Thank you for this!

Fri, Oct 30, 9:41 AM · Restricted Project
aaron.ballman accepted D90336: [Sema] Diagnose annotating `if constexpr` with a likelihood attribute.

LGTM aside from the point @rjmccall made about the note.

Fri, Oct 30, 9:36 AM · Restricted Project
aaron.ballman accepted D90442: [PS4] Support dllimport/export attributes.

LGTM!

Fri, Oct 30, 4:48 AM · Restricted Project

Yesterday

aaron.ballman added a comment to D89743: Support Attr in DynTypedNode and ASTMatchers..

(while this is useful to you, let's keep discussing, but I'm also happy to stop and land this with your preferred API/semantics - just LMK if changes are needed)

Thu, Oct 29, 2:45 PM · Restricted Project
aaron.ballman accepted D90269: adds -Wfree-nonheap-object member var checks.

LGTM!

Thu, Oct 29, 2:13 PM · Restricted Project
aaron.ballman added a comment to D90109: [clang-tidy] Use ANSI escape codes for --use-color on Windows.

What issues did you run into regarding testing, because I feel like the patch should have test coverage (esp given that color vs ANSI escape codes are a bit of an oddity to reason about)?

I'm unable to create a debug build, due to issues with LLVM and Visual Studio 16.7. There's Google results on the issue, but I don't see any solution and I'm using master and the latest version of Visual Studio. It seems you can (and perhaps should?) run the tests with a Release build, so I've gone ahead and done that.

Thu, Oct 29, 12:28 PM · Restricted Project, Restricted Project
aaron.ballman accepted D82278: Fix traversal over CXXConstructExpr in Syntactic mode.

LGTM!

Thu, Oct 29, 12:20 PM · Restricted Project
aaron.ballman added inline comments to D90316: [FPEnv] Diagnose pragmas FENV_ROUND,_ACCESS and float_control if target does not support StrictFP.
Thu, Oct 29, 10:22 AM · Restricted Project

Wed, Oct 28

aaron.ballman added a reviewer for D89743: Support Attr in DynTypedNode and ASTMatchers.: klimek.

Adding @klimek as a reviewer since I forgot to do that last time.

Wed, Oct 28, 12:39 PM · Restricted Project
aaron.ballman accepted D89743: Support Attr in DynTypedNode and ASTMatchers..

I was sort of expecting hasAttr() to be deprecated because it is a leaky abstraction (consumers of the API have to know about our internal naming convention for attribute kind enumerations, it makes it harder for use to change the definition of the enumeration, etc.

Sure, SGTM

I'm totally fine if this happens in a follow-up patch (or patches). WDYT?

I definitely don't agree that hasName() has different semantics for attributes from declarations -- both of them are named identifiers, potentially with scope information as part of the name. Making the matcher polymorphic matches my expectations (pun intended).

Fair enough - this isn't really my area, but the differences I was thinking of were:

  • the Attr uses a name rather than owning it - it's more like DeclRefExpr than like NamedDecl
  • it wasn't clear to me that you'd want the same rules about optional scope handling, though it sounds like you do
Wed, Oct 28, 12:39 PM · Restricted Project
aaron.ballman added inline comments to D90129: Better source location for -Wignored-qualifiers on trailing return types.
Wed, Oct 28, 8:11 AM · Restricted Project
aaron.ballman accepted D90244: [clang-tidy][NFC] IdentifierNaming: Remove unnecessary string allocations.

Was this caused by a performance concern when profiling something? I'm not opposed to the changes, but I do think the original formulation is easier to read.

It's not a huge performance concern, but removing up to 156 malloc calls* for each time we read** or store the style is certainly reason for this.

*Depends on standard library implementations small string optimisation buffer size.
**Happens multiple times as of 4888c9ce.

Wed, Oct 28, 7:14 AM · Restricted Project
aaron.ballman added inline comments to D90269: adds -Wfree-nonheap-object member var checks.
Wed, Oct 28, 6:20 AM · Restricted Project
aaron.ballman added inline comments to D89988: adds basic -Wfree-nonheap-object functionality.
Wed, Oct 28, 6:06 AM · Restricted Project
aaron.ballman accepted D90129: Better source location for -Wignored-qualifiers on trailing return types.

LGTM aside from a question about a potential assert to add.

Wed, Oct 28, 5:52 AM · Restricted Project

Tue, Oct 27

aaron.ballman added inline comments to D89899: [CodeGen] Implement [[likely]] and [[unlikely]] for the iteration statements.
Tue, Oct 27, 12:44 PM · Restricted Project
aaron.ballman added a comment to D90180: [clang-tidy] find/fix unneeded semicolon after switch.

Discussion of change on LKML
https://lkml.org/lkml/2020/10/27/2538

On why the existing clang fixit is not practical.
tl;dr
10,000 changes to look for a treewide fix
The fixit has whitespace issue

Tue, Oct 27, 11:47 AM · Restricted Project, Restricted Project
aaron.ballman accepted D89988: adds basic -Wfree-nonheap-object functionality.

LGTM aside from a few request for comments in the test cases.

Tue, Oct 27, 11:16 AM · Restricted Project
aaron.ballman added a comment to D90244: [clang-tidy][NFC] IdentifierNaming: Remove unnecessary string allocations.

Was this caused by a performance concern when profiling something? I'm not opposed to the changes, but I do think the original formulation is easier to read.

Tue, Oct 27, 11:12 AM · Restricted Project
aaron.ballman added a comment to D90042: [clang-tidy] performance-unnecessary-copy-initialization: Check for const reference arguments that are replaced template parameter type..
In D90042#2356265, @flx wrote:
In D90042#2350035, @flx wrote:

I should note that I was only able to reproduce the false positive with the actual implementation std::function and not our fake version here.

Any reason not to lift enough of the actual definition to be able to reproduce the issue in your test cases? Does the change in definitions break other tests?

I poured over the actual definition and couldn't find any difference wrt the call operator that would explain it. I would also think that:

template <typename T>
void foo(T&& t) {
  std::forward<T>(t).modify();
}

would be a simpler case that should trigger replacement, but it doesn't. Do you have any idea what I could be missing?

Tue, Oct 27, 10:47 AM · Restricted Project, Restricted Project
aaron.ballman added inline comments to D89988: adds basic -Wfree-nonheap-object functionality.
Tue, Oct 27, 10:27 AM · Restricted Project
aaron.ballman added inline comments to D90129: Better source location for -Wignored-qualifiers on trailing return types.
Tue, Oct 27, 10:00 AM · Restricted Project
aaron.ballman added inline comments to D89899: [CodeGen] Implement [[likely]] and [[unlikely]] for the iteration statements.
Tue, Oct 27, 8:15 AM · Restricted Project
aaron.ballman accepted D90110: [clang-tidy] Use --use-color in run-clang-tidy.py.

This change LGTM, but please wait a few days in case @alexfh (or others) have concerns.

Tue, Oct 27, 7:40 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D54943: WIP [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness.

Would it be possible to split this review further, into "checking" and "fixing" portions? I understand that fix-it portion is more difficult, and sometimes results in multiple 'const' keywords being added, but for the past year we have used the check as part of regular CI runs to flag where it needs to be added by the engineers and for our use case it works fine that way - so I would prefer to have at least the "report" part as part of upstream Clang12, even if the 'fixup' comes later, due to the complexity of covering all corner cases.

Tue, Oct 27, 7:39 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D90180: [clang-tidy] find/fix unneeded semicolon after switch.

Is this not already handled by -Wextra-semi. If it isn't I feel like it should be handled there rather than in clang-tidy

Tue, Oct 27, 7:05 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D90109: [clang-tidy] Use ANSI escape codes for --use-color on Windows.

Added a few inline comments for clarification.

Note: I was not able to get a debug build and unit tests working on my Windows set up, so this has only been tested manually. I'm not sure if the unit test added in D79477 runs on Windows, but in theory it should be broken on Windows since it expects ANSI escape codes in the output.

Tue, Oct 27, 6:55 AM · Restricted Project, Restricted Project
aaron.ballman added inline comments to D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.
Tue, Oct 27, 6:15 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D90042: [clang-tidy] performance-unnecessary-copy-initialization: Check for const reference arguments that are replaced template parameter type..
In D90042#2350035, @flx wrote:

I should note that I was only able to reproduce the false positive with the actual implementation std::function and not our fake version here.

Tue, Oct 27, 5:45 AM · Restricted Project, Restricted Project
aaron.ballman updated subscribers of D89743: Support Attr in DynTypedNode and ASTMatchers..

My preference is to add support for hasName() as that's going to be the most common need for matching attributes.

Sounds good (though I ran into issues calling this hasName specifically, see below). There's overlap with hasAttr(Kind) but I think that only handles decls, you can't bind the attribute itself, and it's harder to extend to the other stuff you mentioned.
So maybe hasAttr gets deprecated, or maybe has(attr(hasName("foo"))) is a mouthful for a common case so the shorthand is nice.

Tue, Oct 27, 5:41 AM · Restricted Project

Mon, Oct 26

aaron.ballman added a comment to D89988: adds basic -Wfree-nonheap-object functionality.

Doing fun tricks with the CFG sounds like a reasonable next step here, and doing so will likely be way more powerful (+ CPU-consuming, and invite potentially more false-positives). I agree that that probably shouldn't be under -Wall, but I wonder if these trivial cases aren't worth putting there on their own? It's not a perfect parallel, but our split between -Wuninitialized + -Wsometimes-uninitialized comes to mind.

Mon, Oct 26, 1:19 PM · Restricted Project
aaron.ballman added a comment to D89988: adds basic -Wfree-nonheap-object functionality.

In order to avoid false negatives, I think this needs to be flow-sensitive to track values. e.g., I would love for this to catch code like:

void func(bool b) {
  int i = 12;
  int *ip = b ? &i : (int *)malloc(sizeof(int));

  ...

  free(ip); // Should we warn or not?
}

While this example is incredibly contrived, you'll fine real-world code does this sort of path sensitive stuff relatively often (a common example is a function that sometimes returns a value that's been strdup()'ed except for one bad function that returns a string literal).

Yep, this is what I'm considering "phase 2" or "phase 3". Here's another one of note:

char* p = static_cast<char*>(std::malloc(8));
char& r = *p;
std::free(&p); // should warn
std::free(&r); // shouldn't warn

A key intention of D89988 is to get minimal coverage and then expand from there.

Mon, Oct 26, 11:06 AM · Restricted Project
aaron.ballman added a comment to D89988: adds basic -Wfree-nonheap-object functionality.

In order to avoid false negatives, I think this needs to be flow-sensitive to track values. e.g., I would love for this to catch code like:

void func(bool b) {
  int i = 12;
  int *ip = b ? &i : (int *)malloc(sizeof(int));
Mon, Oct 26, 10:45 AM · Restricted Project

Sun, Oct 25

aaron.ballman accepted D84604: Thread safety analysis: Consider global variables in scope.

LGTM

Sun, Oct 25, 6:22 AM · Restricted Project

Sat, Oct 24

aaron.ballman added inline comments to D90073: [Clang][CodeGen] fix failed assertion.
Sat, Oct 24, 6:45 AM · Restricted Project

Thu, Oct 22

aaron.ballman added a comment to D88645: [Annotation] Allows annotation to carry some additional constant arguments..

LGTM aside from a request for a comment to be added. Thank you!

do you mean an RFC on llvm-dev/cfe-dev ?

Thu, Oct 22, 9:45 AM · Restricted Project, Restricted Project
aaron.ballman added inline comments to D86671: [clang-tidy] Add new case type to check variables with Hungarian notation.
Thu, Oct 22, 8:23 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D89899: [CodeGen] Implement [[likely]] and [[unlikely]] for the iteration statements.

Thank you for this!

Thu, Oct 22, 7:44 AM · Restricted Project
aaron.ballman requested changes to D72553: [clang-tidy] Add performance-prefer-preincrement check.

could this do with an alias into performance?

Thu, Oct 22, 7:32 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D89651: [clang-tidy] Add bugprone-suspicious-memory-comparison check.

Should point out there is already a check for cert-oop57-cpp, added in D72488. Do these play nice with each other or should they perhaps be merged or share code?

I would certainly expect some duplicate warnings with there two checks, but as far as I can tell that check does not currently warn on using memcmp with non-standard-layout types.
I personally would leave the exp42 and flp37 parts of this check as separate, because those are useful in C as well. But maybe it'd be better to move the warning for non-standard-layout types from here to the existing one.

Thu, Oct 22, 7:03 AM · Restricted Project, Restricted Project
aaron.ballman accepted D88645: [Annotation] Allows annotation to carry some additional constant arguments..

LGTM aside from a request for a comment to be added. Thank you!

Thu, Oct 22, 6:15 AM · Restricted Project, Restricted Project
aaron.ballman added inline comments to D85697: [clang-tidy] Add cppcoreguidelines-prefer-scoped-enums.
Thu, Oct 22, 5:36 AM · Restricted Project, Restricted Project

Wed, Oct 21

aaron.ballman accepted D89332: [clang-tidy] performance-unnecessary-copy-initialization: Always allow std::function to be copied..

LGTM with an extra testing request (that should hopefully just work for you).

Wed, Oct 21, 11:37 AM · Restricted Project
aaron.ballman accepted D88112: Clang is crashing after generating the right diagnostic for a re-declaration of a friend method - Fix for PR47544.

LGTM aside from some small nits. You should wait a day or two before landing in case @rnk or other reviewers have further comments.

Wed, Oct 21, 11:32 AM
aaron.ballman added a comment to D74299: [clang-tidy] extend tests of run-clang-tidy.

I cannot reproduce the problem and there is just not enough info to go on.

Wed, Oct 21, 6:59 AM · Restricted Project

Tue, Oct 20

aaron.ballman added inline comments to D89490: Introduce __attribute__((darwin_abi)).
Tue, Oct 20, 11:54 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D85697: [clang-tidy] Add cppcoreguidelines-prefer-scoped-enums.

I tend to be very skeptical of the value of checks that basically throw out entire usable chunks of the base language because the check is effectively impossible to apply to an existing code base. Have you run the check over large C++ code bases to see just how often the diagnostic triggers and whether there are ways we might want to reduce false-positives that the C++ Core Guidelines haven't considered as part of their enforcement strategy?

I disagree with this mentality, This is following what the core guideline states perfectly. If a codebase doesn't want to adhere to those guidelines, they don't need to run this check. If you think there are shortcomings in the guidelines, raise an issue on their github.

Tue, Oct 20, 10:55 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D89743: Support Attr in DynTypedNode and ASTMatchers..

This is *awesome*, thank you so much for working on it!

Thanks for the comments - haven't addressed them yet, but wanted to clarify scope first.

One question I have is: as it stands, this is not a particularly useful matcher because it can only be used to say "yup, there's an attribute"

Yes definitely, I should have mentioned this...

My main goal here was to support it in DynTypedNode, as we have APIs (clangd::SelectionTree) that can only handle nodes that fit there.
But the common test pattern in ASTTypeTraitsTest used matchers, and I figured a basic matcher wasn't hard to add.

Tue, Oct 20, 10:22 AM · Restricted Project
aaron.ballman added a comment to D89638: [Analyzer][WebKit] Add attributes to suppress specific checkers.

I was thinking about an alternative approach (5th approach (?), yeah, I think we need an RFC).

What if we had just one attribute with a StringArgument ?
Actually, we already have that with the annotate attribute.
How do you like this?

struct [[clang::annotate("CSA:supress:RefCntblBaseVirtualDtor")]] Derived2
    : RefCntblBase{};

Disadvantages: we must process strings whenever a node has the annotate attr attached, we have to come up with a "DSL".
Advantages: total flexibility.
WDYT?

Tue, Oct 20, 8:09 AM
aaron.ballman added reviewers for D88112: Clang is crashing after generating the right diagnostic for a re-declaration of a friend method - Fix for PR47544: rsmith, rjmccall.

Adding a few more reviewers in case I've got something wrong.

Tue, Oct 20, 7:45 AM
aaron.ballman requested changes to D89743: Support Attr in DynTypedNode and ASTMatchers..
Tue, Oct 20, 7:40 AM · Restricted Project
aaron.ballman added a reviewer for D89743: Support Attr in DynTypedNode and ASTMatchers.: aaron.ballman.

This is *awesome*, thank you so much for working on it! One question I have is: as it stands, this is not a particularly useful matcher because it can only be used to say "yup, there's an attribute" -- are you planning to extend this functionality so that you can do something like attr(hasName("foo")), or specify the syntax the attribute uses, isImplicit(), etc?

Tue, Oct 20, 7:40 AM · Restricted Project

Mon, Oct 19

aaron.ballman added inline comments to D89638: [Analyzer][WebKit] Add attributes to suppress specific checkers.
Mon, Oct 19, 9:47 AM
aaron.ballman added a comment to D85697: [clang-tidy] Add cppcoreguidelines-prefer-scoped-enums.

I tend to be very skeptical of the value of checks that basically throw out entire usable chunks of the base language because the check is effectively impossible to apply to an existing code base. Have you run the check over large C++ code bases to see just how often the diagnostic triggers and whether there are ways we might want to reduce false-positives that the C++ Core Guidelines haven't considered as part of their enforcement strategy?

Mon, Oct 19, 9:07 AM · Restricted Project, Restricted Project
aaron.ballman requested changes to D89638: [Analyzer][WebKit] Add attributes to suppress specific checkers.
Mon, Oct 19, 8:18 AM
aaron.ballman added inline comments to D89649: Fix __has_unique_object_representations with no_unique_address.
Mon, Oct 19, 8:11 AM · Restricted Project
aaron.ballman accepted D72218: [clang-tidy] new altera kernel name restriction check.

Addressed comments by @aaron.ballman regarding the diagnostic warning.

I tried to add a test case for when the filename is kernel.cl, verilog.cl, or vhdl.cl, but that did not work because the test suite appends .tmp.cpp to the end of the test files, and kernel.cl.tmp.cpp is not a restricted filename. If you know of a way to include this test case in the test suite, please let me know.

Mon, Oct 19, 8:08 AM · Restricted Project, Restricted Project
aaron.ballman accepted D89407: [clang-tidy] Add scoped enum constants to identifier naming check.

LGTM aside from a minor nit.

Mon, Oct 19, 7:41 AM · Restricted Project
aaron.ballman added a comment to D74299: [clang-tidy] extend tests of run-clang-tidy.

Could someone commit this as I cannot? Thanks a lot!
Alexander Lanin <alex@lanin.de>

Thanks for review @aaron.ballman!

Happy to do so! I've commit on your behalf in 627c01bee0deb353b3e3e90c1b8d0b6d73464466

Mon, Oct 19, 7:38 AM · Restricted Project
aaron.ballman added a reverting change for rG627c01bee0de: Extend tests of run-clang-tidy: rGb91a236ee1c3: Revert "Extend tests of run-clang-tidy".
Mon, Oct 19, 7:38 AM
aaron.ballman committed rGb91a236ee1c3: Revert "Extend tests of run-clang-tidy" (authored by aaron.ballman).
Revert "Extend tests of run-clang-tidy"
Mon, Oct 19, 7:38 AM
aaron.ballman closed D74299: [clang-tidy] extend tests of run-clang-tidy.

Could someone commit this as I cannot? Thanks a lot!
Alexander Lanin <alex@lanin.de>

Thanks for review @aaron.ballman!

Mon, Oct 19, 7:29 AM · Restricted Project
aaron.ballman committed rG627c01bee0de: Extend tests of run-clang-tidy (authored by AlexanderLanin).
Extend tests of run-clang-tidy
Mon, Oct 19, 7:29 AM
aaron.ballman added inline comments to D89490: Introduce __attribute__((darwin_abi)).
Mon, Oct 19, 7:24 AM · Restricted Project, Restricted Project
aaron.ballman accepted D74299: [clang-tidy] extend tests of run-clang-tidy.

LGTM!

Mon, Oct 19, 7:07 AM · Restricted Project
aaron.ballman added inline comments to D89212: PR47663: Warn if an entity becomes weak after its first use..
Mon, Oct 19, 7:05 AM · Restricted Project
aaron.ballman added a comment to D89332: [clang-tidy] performance-unnecessary-copy-initialization: Always allow std::function to be copied..

Come to think of it, this is a pretty illogical way to solve this problem, just append ::std::function to the AllowedTypes vector in registerMatchers and be do with it. Will require dropping the const Qualifier on AllowedTypes, but aside from that it is a much simpler fix.
The has(Any)Name matcher has logic for skipping inline namespaces.

Mon, Oct 19, 6:52 AM · Restricted Project
aaron.ballman added inline comments to D87449: [clang-tidy] Add new check for SEI CERT rule SIG30-C.
Mon, Oct 19, 6:49 AM · Restricted Project, Restricted Project
aaron.ballman added inline comments to D88645: [Annotation] Allows annotation to carry some additional constant arguments..
Mon, Oct 19, 6:22 AM · Restricted Project, Restricted Project

Thu, Oct 15

aaron.ballman accepted D89210: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in SwitchStmt.

LGTM aside from a documentation request, but you may want to wait a few days before committing in case Richard or John have opinions.

Thu, Oct 15, 6:33 AM · Restricted Project

Wed, Oct 14

aaron.ballman accepted D89212: PR47663: Warn if an entity becomes weak after its first use..

LGTM aside from some very small nits (feel free to ignore any that don't make sense to you).

Wed, Oct 14, 5:39 AM · Restricted Project

Tue, Oct 13

aaron.ballman added a comment to D89210: [Sema, CodeGen] Implement [[likely]] and [[unlikely]] in SwitchStmt.

Thank you for the continued work on this feature! I'd like to better understand the behavior of fallthrough labels because I think there are two conceptual models we could follow. When a label falls through to another label, should both labels be treated as having the same likelihood or should they be treated as distinct cases? e.g.,

switch (something) {
case 1:
  ...
[[likely]] case 2:
  ...
  break;
case 3:
  ...

Should case 1 be considered likely because it falls through to case 2? From the patch, it looks like your answer is "yes", they should both be likely, but should that hold true even if case 1 does a lot of work and is not actually all that likely? Or is the user expected to write [[unlikely]] case 1: in that situation? We don't seem to have a test case for a situation where fallthrough says something like that:

switch (something) {
[[likely]] case 0:
  ...
  // Note the fallthrough
[[unlikely]] case 1:
  ...
}
Tue, Oct 13, 12:29 PM · Restricted Project
aaron.ballman added inline comments to D54943: WIP [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness.
Tue, Oct 13, 11:10 AM · Restricted Project, Restricted Project
aaron.ballman added inline comments to D67422: [analyzer] NFC: Move path diagnostic consumer implementations to libAnalysis..
Tue, Oct 13, 10:58 AM · Restricted Project
aaron.ballman added a comment to D88314: Added llvm-string-referencing check.

@aaron.ballman Thank you for picking up this review! Running the check over the entire LLVM causes ~74K warnings across 430 files. As to the false positive rate it's tricky to measure. Based on previous analysis on flang codebase, I would say roughly 50% of the fixes would cause compiler errors when applied.

Tue, Oct 13, 10:51 AM · Restricted Project

Fri, Oct 9

aaron.ballman added a comment to D88645: [Annotation] Allows annotation to carry some additional constant arguments..

(Drive By: This is cool)

Fri, Oct 9, 1:02 PM · Restricted Project, Restricted Project
aaron.ballman added a comment to D88314: Added llvm-string-referencing check.

It looks like you generated a diff from a previous diff instead of trunk -- can you regenerate the diff against trunk so that reviewers can see the full content of the changes?

Fri, Oct 9, 12:40 PM · Restricted Project
aaron.ballman added inline comments to D54943: WIP [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness.
Fri, Oct 9, 12:38 PM · Restricted Project, Restricted Project
aaron.ballman added inline comments to D87449: [clang-tidy] Add new check for SEI CERT rule SIG30-C.
Fri, Oct 9, 11:08 AM · Restricted Project, Restricted Project
aaron.ballman added inline comments to D87449: [clang-tidy] Add new check for SEI CERT rule SIG30-C.
Fri, Oct 9, 5:24 AM · Restricted Project, Restricted Project
aaron.ballman accepted D87279: [clang] Fix handling of physical registers in inline assembly operands..

This LGTM, but I'm not super well-versed with asm statements to begin with. You should wait a bit for other reviewers to weigh in before landing.

Fri, Oct 9, 5:20 AM · Restricted Project

Thu, Oct 8

aaron.ballman added a comment to D88314: Added llvm-string-referencing check.

Thank you for working on this check! Have you run the check over LLVM to see how much it reports? One of the concerns I have with this is that it's not always appropriate to replace a const std::string& with an llvm::StringRef and so I'm wondering what the "false positive" rate is for the check. For instance, there are std::string member functions that don't exist on StringRef so the fix-it will introduce compile errors, or switching the parameter type will cause additional unnecessary conversions.

Thu, Oct 8, 11:52 AM · Restricted Project
aaron.ballman added a comment to D88833: [clang-tidy] Do not warn on pointer decays in system macros.

I think the current behavior should instead be configurable, with a way to opt-in into weaker guarantees.

Thu, Oct 8, 10:58 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D88833: [clang-tidy] Do not warn on pointer decays in system macros.

@aaron.ballman valid point. It would seem natural to diagnose both since the user is voluntarily causing the decay?

Thu, Oct 8, 10:31 AM · Restricted Project, Restricted Project
aaron.ballman added inline comments to D87279: [clang] Fix handling of physical registers in inline assembly operands..
Thu, Oct 8, 9:17 AM · Restricted Project
aaron.ballman added inline comments to D54943: WIP [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness.
Thu, Oct 8, 7:49 AM · Restricted Project, Restricted Project
aaron.ballman accepted D88088: [clang] improve accuracy of ExprMutAnalyzer.

LGTM though I may have spotted a potential improvement (it can be handled in a follow-up if you want).

Thu, Oct 8, 7:30 AM · Restricted Project
aaron.ballman added a comment to D69560: [clang-tidy] Add 'experimental-cppcoreguidelines-avoid-adjacent-parameters-of-the-same-type' check.

I'd like to resurrect the discussion about this. Unfortunately, the concept of experimental- group (in D76545) has fallen silent, too. We will present the results of this analysis soon at IEEE SCAM (Source Code Analysis and Manipulation) 2020. While a previous submission about this topic was rejected on the grounds of not being in line with the conference's usual, hyper-formalised nature, with one reviewer especially praising the paper for its nice touch with the empirical world and the fact that neither argument swaps (another checker worked on by a colleague) nor the interactions of this issue with the type system (this checker) was really investigated in the literature for C++ before, we've tailored both the paper and the implementation based on reviewer comments from the scientific world, and the comments here.
The reviews were mostly favourable, excl. comments about the lack of formalisation and the unfortunate lack of modelling for template instantiations. Such a cold cut, however, only introduces false negatives into the result set. Which is fine, as the users will never see those non-existent reports!

Thu, Oct 8, 6:58 AM · Restricted Project, Restricted Project
aaron.ballman added inline comments to D88645: [Annotation] Allows annotation to carry some additional constant arguments..
Thu, Oct 8, 6:00 AM · Restricted Project, Restricted Project

Wed, Oct 7

aaron.ballman added inline comments to D87449: [clang-tidy] Add new check for SEI CERT rule SIG30-C.
Wed, Oct 7, 7:32 AM · Restricted Project, Restricted Project
aaron.ballman added inline comments to D87449: [clang-tidy] Add new check for SEI CERT rule SIG30-C.
Wed, Oct 7, 6:41 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D88833: [clang-tidy] Do not warn on pointer decays in system macros.

Despite my earlier happiness with the patch, it's now a bit unclear to me from the C++ Core Guideline wording what the right behavior here actually is. The bounds profile is trying to ensure you don't access out-of-range elements of a container and the decay part specifically seems to be more about how it impacts pointer arithmetic.

Wed, Oct 7, 6:28 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D88275: [ASTMatchers] Add matcher `hasParentIgnoringImplicit`..

I'm not concerned about the basic idea behind the proposed matcher, I'm only worried we're making AST matching more confusing by having two different ways of inconsistently accomplishing the same-ish thing.

Aaron, I appreciate this concern, but I would argue that this matcher isn't making things any worse. We already have the various ignoringImplicit matchers, and this new one simply parallels those, but for parents. So, it is in some sense "completing" an existing API, which together is an alternative to traverse.

I'm not certain I agree with that reasoning because you can extend it to literally any match that may interact with implicit nodes, which is the whole point to the spelled in source traversal mode. I'm not certain it's a good design for us to continue to add one-off ways to ignore implicit nodes.

I appreciate your point, but I'm personally inclined to allow progress while we figure these larger issues out. That said, I'm in no rush and would like a solution that we're both happy with. How do you propose we proceed, especially given that traverse does not currently support this case? It seems that progress is in part blocked on hearing back from steveire, but it's been over a week since you added him to the review thread. Is there some other way to ping him?

Wed, Oct 7, 5:33 AM · Restricted Project

Tue, Oct 6

aaron.ballman closed D87962: [clang] Change the multi-character character constants from extension to implementation-defined..

LGTM! Do you need someone to commit on your behalf?

Yes I need!

Tue, Oct 6, 5:48 AM · Restricted Project
aaron.ballman committed rG8fa45e1fd527: Convert diagnostics about multi-character literals from extension to warning (authored by nomanous).
Convert diagnostics about multi-character literals from extension to warning
Tue, Oct 6, 5:47 AM

Mon, Oct 5

aaron.ballman added a comment to D88831: [clang-tidy] Remove obsolete checker google-runtime-references.

One request though: can you add a release note about the removal with a brief explanation of why it was removed?

Mon, Oct 5, 2:33 PM · Restricted Project, Restricted Project
aaron.ballman accepted D87962: [clang] Change the multi-character character constants from extension to implementation-defined..

LGTM! Do you need someone to commit on your behalf?

Mon, Oct 5, 12:56 PM · Restricted Project
aaron.ballman accepted D88831: [clang-tidy] Remove obsolete checker google-runtime-references.

LGTM to remove rather than move into readability unless someone has a good argument as to why it belongs there. I don't think this pattern makes code more readable in general as it recommends using a more verbose syntax (pointers, which require dereferencing) over a safer, concise syntax (references, which carry semantic information about nullness).

Mon, Oct 5, 12:53 PM · Restricted Project, Restricted Project