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 (358 w, 5 d)

Recent Activity

Today

aaron.ballman added inline comments to D73548: [clang-tidy] Added option for disabling const qualifiers in readability-qualified-auto.
Tue, Jan 28, 1:45 PM · Restricted Project, Restricted Project
aaron.ballman added a comment to D73464: [clang] Add TagDecl AST matcher.

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

Yes, please. I don`t know how :)

Tue, Jan 28, 1:22 PM · Restricted Project
aaron.ballman added a comment to D73457: [Clang] Warn about 'z' printf modifier in old MSVC..

Since we auto-detect -fmsc-version if it's not explicitly installed and since this warning is on by default, it makes the test suite depend on the environment a good bit. Given how old 2015 is by now, I'm not sure this complexity and subtlety is worth the benefit?

Tue, Jan 28, 1:22 PM · Restricted Project
aaron.ballman added inline comments to D73562: [ASTMatchers] Add hasPlacementArg and hasAnyPlacementArg traversal matcher for CXXNewExpr.
Tue, Jan 28, 1:11 PM · Restricted Project
aaron.ballman committed rG554791928088: Fix a crash when casting _Complex and ignoring the results. (authored by aaron.ballman).
Fix a crash when casting _Complex and ignoring the results.
Tue, Jan 28, 10:12 AM
aaron.ballman closed D73545: Fix a crash when casting _Complex and ignoring the results.

Committed in 554791928088d6139e0fb3480d79cd76ea59198f

Tue, Jan 28, 10:11 AM
aaron.ballman added inline comments to D72282: [clang-tidy] Add `bugprone-unintended-adl`.
Tue, Jan 28, 5:36 AM · Restricted Project, Restricted Project
aaron.ballman created D73545: Fix a crash when casting _Complex and ignoring the results.
Tue, Jan 28, 5:36 AM
aaron.ballman added a comment to D72217: [clang-tidy] Added readability-qualified-auto check.

https://llvm.org/docs/CodingStandards.html#beware-unnecessary-copies-with-auto

// Copy pointers, but make it clear that they're pointers.
for (const auto *Ptr : Container) { observe(*Ptr); }
for (auto *Ptr : Container) { Ptr->change(); }

This is the reasoning behind putting the const qualifier in the check. If people feel that this isn't quite enough to justify the const qualifier I'll submit a follow up. The general consensus though is that you should mark auto pointers as const if they don't need to change to express intent

The text/rule there is explicitly about avoiding/clarifying copies - the examples indeed use 'const' but AFAICT the "don't copy" reasoning only applies to including *&.

FWIW I think const here is often noise, particularly in AST-walking code where you're traversing an edge from an X* to a Y* - the latter will be const if the former is, and I care at API boundaries but not in between. (It's usually a meaningless distinction - e.g. we're just reading but it's a non-const pointer because RecursiveASTVisitor isn't const-friendly).

So while spelling const is often helpful, we shouldn't (and don't) require it, and the current config of this check is too intrusive.

Tue, Jan 28, 5:18 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D72448: [clang-tidy] readability-redundant-string-init now flags redundant initialisation in Field Decls and Constructor Initialisers.

Hmm a lot of the code in the redundant-string-init check is designed to be macro unsafe. Not sure the best way to follow up, discard the old macro behaviour or keep it

Designed to be macro unsafe, or just didn't consider macros in the first place? I'm not seeing anything that makes me think macros were taken into account, but maybe you're looking at something different from me. Do you have an example of where you think something was designed to be macro unsafe?

This is one of the test cases

#define M(x) x
#define N { std::string s = ""; }
// CHECK-FIXES: #define N { std::string s = ""; }

void h() {
  templ<int>();
  templ<double>();

  M({ std::string s = ""; })
  // CHECK-MESSAGES: [[@LINE-1]]:19: warning: redundant string initialization
  // CHECK-FIXES: M({ std::string s; })

  N
  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: redundant string initialization
  // CHECK-FIXES: N
  N
  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: redundant string initialization
  // CHECK-FIXES: N
}
// further down
#define EMPTY_STR ""
void j() {
  std::string a(EMPTY_STR);
  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: redundant string initialization
  // CHECK-FIXES: std::string a;
  std::string b = (EMPTY_STR);
  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: redundant string initialization
  // CHECK-FIXES: std::string b;

Looks like they knew the behaviour they wanted back then

Tue, Jan 28, 5:18 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D72448: [clang-tidy] readability-redundant-string-init now flags redundant initialisation in Field Decls and Constructor Initialisers.

Hmm a lot of the code in the redundant-string-init check is designed to be macro unsafe. Not sure the best way to follow up, discard the old macro behaviour or keep it

Tue, Jan 28, 4:59 AM · Restricted Project, Restricted Project
aaron.ballman accepted D73464: [clang] Add TagDecl AST matcher.

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

Tue, Jan 28, 4:59 AM · Restricted Project

Yesterday

aaron.ballman accepted D72448: [clang-tidy] readability-redundant-string-init now flags redundant initialisation in Field Decls and Constructor Initialisers.

LGTM!

Mon, Jan 27, 1:49 PM · Restricted Project, Restricted Project
aaron.ballman accepted D73434: [Sema] Remove a -Wrange warning from -Wall.

LGTM!

Mon, Jan 27, 1:48 PM · Restricted Project
aaron.ballman accepted D73457: [Clang] Warn about 'z' printf modifier in old MSVC..

Removed the special case for MSCompatibilityVersion == 0. If the default compatibility setting needs to be changed, that's a separate piece of work and should be done by someone who understands more than I do about all those failing tests :-) The new version of this patch just uses the existing default.

+1. The issue of how the default version gets set is a separate issue. I think it's best to keep this patch decoupled from that.

Mon, Jan 27, 10:57 AM · Restricted Project
aaron.ballman added reviewers for D73457: [Clang] Warn about 'z' printf modifier in old MSVC.: rnk, majnemer, zturner.

Adding some more Windows reviewers to see if there are more opinions on how to handle this.

Mon, Jan 27, 9:32 AM · Restricted Project
aaron.ballman added inline comments to D73457: [Clang] Warn about 'z' printf modifier in old MSVC..
Mon, Jan 27, 7:53 AM · Restricted Project
aaron.ballman accepted D73439: [ASTMatchers] Add cxxNoexceptExpr AST matcher.

LGTM aside from some minor nits.

Mon, Jan 27, 7:25 AM · Restricted Project
aaron.ballman added a comment to D73464: [clang] Add TagDecl AST matcher.

You should also regenerate the AST matcher documentation by running clang\docs\tools\dump_ast_matchers.py

Mon, Jan 27, 7:16 AM · Restricted Project
aaron.ballman accepted D73098: [clang-tidy] readability-identifier-naming disregards parameters restrictions on main like functions.
Mon, Jan 27, 6:58 AM · Restricted Project, Restricted Project

Sat, Jan 25

aaron.ballman added a comment to D72810: [LifetimeAnalysis] Add support for lifetime annotations on functions.

Thank you for the work on this!

Sat, Jan 25, 12:14 PM · Restricted Project
aaron.ballman added a comment to D73007: [Sema] Avoid Wrange-loop-analysis false positives.

Here is a proposal: we add two children to -Wrange-loop-analysis.

  • -Wrange-loop-construct warns about possibly unintended constructor calls. This might be in -Wall. It contains
    • warn_for_range_copy: loop variable A of type B creates a copy from type C
    • warn_for_range_const_reference_copy: loop variable A is initialized with a value of a different type resulting in a copy
  • -Wrange-loop-bind-ref[erence] warns about misleading use of reference types. This might not be in -Wall. It contains
    • warn_for_range_variable_always_copy: loop variable A is always a copy because the range of type B does not return a reference
Sat, Jan 25, 11:44 AM · Restricted Project
aaron.ballman accepted D73098: [clang-tidy] readability-identifier-naming disregards parameters restrictions on main like functions.

LGTM aside from some minor nits.

Sat, Jan 25, 11:38 AM · Restricted Project, Restricted Project
aaron.ballman added inline comments to D72448: [clang-tidy] readability-redundant-string-init now flags redundant initialisation in Field Decls and Constructor Initialisers.
Sat, Jan 25, 11:29 AM · Restricted Project, Restricted Project
aaron.ballman added inline comments to D73090: [clang-tidy] Fix PR#44528 'modernize-use-using and enums'.
Sat, Jan 25, 11:20 AM · Restricted Project, Restricted Project
aaron.ballman accepted D65994: Extended FPOptions with new attributes.

LGTM, but I'll echo what @rjmccall said about this level of granularity being a bit too fine for review for future patches (we expect changes to be testable, which this is, but only if you also review other patches and notice the tests and how they relate to this patch, which can be easy for reviewers to miss).

Sat, Jan 25, 11:16 AM · Restricted Project
aaron.ballman added a comment to D72378: [clang-tidy] Add `bugprone-reserved-identifier`.

So, i'm seeing an issue here:
https://godbolt.org/z/KM2qLa

I can't NOLINT it, because it is defined not in the source file, but in compile line.
And i can't whitelist it since there is no whitelist..

PTAL.

Doesn't the AllowedIdentifiers option work for you?

Uuuh. To be honest somehow i did not observe it when looking through
the docs (as indicated in it since there is no whitelist..),
It does work in this case, thanks. Sorry for false alarm.

Sat, Jan 25, 9:13 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D72378: [clang-tidy] Add `bugprone-reserved-identifier`.

So, i'm seeing an issue here:
https://godbolt.org/z/KM2qLa

I can't NOLINT it, because it is defined not in the source file, but in compile line.
And i can't whitelist it since there is no whitelist..

PTAL.

Sat, Jan 25, 8:55 AM · Restricted Project, Restricted Project

Fri, Jan 24

aaron.ballman added a comment to D73028: Extract ExprTraversal class from Expr.

The follow-up is here: https://reviews.llvm.org/D73029 .

I have the need to change the ascending traversal implemented in ASTContext.cpp with a map (populated while descending) to make it skip nodes too during parent traversal.

I didn't want to have descending traversal in Expr.cpp and ascending traversal in ASTContext.cpp as they would be likely to go out of sync, so moving this implementation here allows extending the purpose in the follow-up.

The ascending traversal should not be in the AST library at all. If tooling or static analysis wants this, that's fine, but navigating upwards is not something the AST supports, and we do not want any part of clang proper developing a reliance on having a parent map, so we're working on moving the parent map out of ASTContext (with a plan to eventually move it out of the AST library). See D71313 for related work in this area.

Fri, Jan 24, 1:25 PM · Restricted Project
aaron.ballman added a comment to D71566: New checks for fortified sprintf.

Continues to LG, do you need me to commit on your behalf?

Fri, Jan 24, 12:27 PM · Restricted Project
aaron.ballman added a comment to D73037: Add a way to set traversal mode in clang-query.

There should also be a mention of this in the release notes (especially if the default behavior winds up changing).

Fri, Jan 24, 11:57 AM · Restricted Project
aaron.ballman added a comment to D73029: Extend ExprTraversal class with acending traversal.

Test cases?

Fri, Jan 24, 11:48 AM · Restricted Project
aaron.ballman added a comment to D73028: Extract ExprTraversal class from Expr.

This looks like it's an NFC patch where the only change is to hoist this functionality out into its own interface. Is that correct? If so, can you please be sure to add NFC to the commit log?

Fri, Jan 24, 11:38 AM · Restricted Project
aaron.ballman added a comment to D73007: [Sema] Avoid Wrange-loop-analysis false positives.

I'm in favor of splitting the warning into subgroups, then deciding which ones should be in -Wall. We've done this with other warnings such as the conversion warnings and tautological compare warnings, with only a subset of them in -Wall.

Fri, Jan 24, 4:44 AM · Restricted Project

Thu, Jan 23

aaron.ballman accepted D71566: New checks for fortified sprintf.

(There are still some minor whitespace nits to resolve as well.)

Strange, everything is passed through clang-format-diff :-/

Thu, Jan 23, 6:03 AM · Restricted Project
aaron.ballman added inline comments to D73098: [clang-tidy] readability-identifier-naming disregards parameters restrictions on main like functions.
Thu, Jan 23, 5:35 AM · Restricted Project, Restricted Project

Wed, Jan 22

aaron.ballman committed rG84c5f1963700: Extend misc-misplaced-const to detect using declarations as well as typedef (authored by AlexanderLanin).
Extend misc-misplaced-const to detect using declarations as well as typedef
Wed, Jan 22, 12:33 PM
aaron.ballman added a comment to D72373: [clang-tidy] extend misc-misplaced-const to detect using besides typedef.

Updated misc-misplaced-const.c to reflect new output and fixed one wrong col in misc-misplaced-const.cpp - I really do not understand how that happened.
Rebased and verified with check-clang-tools.

Wed, Jan 22, 12:32 PM · Restricted Project, Restricted Project
aaron.ballman committed rG8edf037aebdf: Add a bit of documentation on attribute spellings that were missing. (authored by aaron.ballman).
Add a bit of documentation on attribute spellings that were missing.
Wed, Jan 22, 12:24 PM
aaron.ballman committed rG90cfbb81674e: Add LLVM_VALUE_FUNCTION to Optional::map(); NFC (authored by aaron.ballman).
Add LLVM_VALUE_FUNCTION to Optional::map(); NFC
Wed, Jan 22, 11:28 AM
aaron.ballman added inline comments to rGdfe9f130e07c: Revert "Unconditionally enable lvalue function designators; NFC".
Wed, Jan 22, 11:00 AM
aaron.ballman committed rG1e4764e10324: Add a comment about when we can remove this construct; NFC. (authored by aaron.ballman).
Add a comment about when we can remove this construct; NFC.
Wed, Jan 22, 10:24 AM
aaron.ballman committed rGdfe9f130e07c: Revert "Unconditionally enable lvalue function designators; NFC" (authored by aaron.ballman).
Revert "Unconditionally enable lvalue function designators; NFC"
Wed, Jan 22, 9:46 AM
aaron.ballman added a reverting change for rG968561bcdc34: Unconditionally enable lvalue function designators; NFC: rGdfe9f130e07c: Revert "Unconditionally enable lvalue function designators; NFC".
Wed, Jan 22, 9:46 AM
aaron.ballman closed D72948: Unconditionally enable lvalue function designators; NFC.

I've commit in 968561bcdc34c7d74482fe3bb69a045abf08d2c1 (clang) and 968561bcdc34c7d74482fe3bb69a045abf08d2c1 (llvm) and will keep an eye on the bots to revert if needed. Thanks!

Wed, Jan 22, 7:12 AM · Restricted Project
aaron.ballman committed rG968561bcdc34: Unconditionally enable lvalue function designators; NFC (authored by aaron.ballman).
Unconditionally enable lvalue function designators; NFC
Wed, Jan 22, 7:03 AM
aaron.ballman added a comment to D73098: [clang-tidy] readability-identifier-naming disregards parameters restrictions on main like functions.

Does wmain get used in a lot of projects. If it's very niche then I don't feel it warrants a place in here. If it does I'll add it in.

Wed, Jan 22, 6:52 AM · Restricted Project, Restricted Project
aaron.ballman committed rGe3b15ed376f3: Revert "Extend misc-misplaced-const to detect using declarations as well as… (authored by aaron.ballman).
Revert "Extend misc-misplaced-const to detect using declarations as well as…
Wed, Jan 22, 6:15 AM
aaron.ballman added a reverting change for rGecc7dae50c41: Extend misc-misplaced-const to detect using declarations as well as typedef: rGe3b15ed376f3: Revert "Extend misc-misplaced-const to detect using declarations as well as….
Wed, Jan 22, 6:15 AM
aaron.ballman added a comment to D72373: [clang-tidy] extend misc-misplaced-const to detect using besides typedef.

Looks like this breaks tests: http://45.33.8.238/linux/8102/step_8.txt

Wed, Jan 22, 6:14 AM · Restricted Project, Restricted Project
aaron.ballman committed rGecc7dae50c41: Extend misc-misplaced-const to detect using declarations as well as typedef (authored by AlexanderLanin).
Extend misc-misplaced-const to detect using declarations as well as typedef
Wed, Jan 22, 5:47 AM
aaron.ballman closed D72373: [clang-tidy] extend misc-misplaced-const to detect using besides typedef.

I've commit on your behalf in ecc7dae50c41bc8a129a158ecf0ae0270126505c. Sorry about the delay in committing and thank you for the patch!

Wed, Jan 22, 5:47 AM · Restricted Project, Restricted Project

Tue, Jan 21

aaron.ballman added inline comments to D73007: [Sema] Avoid Wrange-loop-analysis false positives.
Tue, Jan 21, 2:14 PM · Restricted Project
aaron.ballman added inline comments to D73098: [clang-tidy] readability-identifier-naming disregards parameters restrictions on main like functions.
Tue, Jan 21, 2:05 PM · Restricted Project, Restricted Project
aaron.ballman added a comment to D71566: New checks for fortified sprintf.

(There are still some minor whitespace nits to resolve as well.)

Tue, Jan 21, 1:56 PM · Restricted Project
aaron.ballman committed rGa8c2f76cd258: Removing an accidentally duplicated line of test code to fix bots. (authored by aaron.ballman).
Removing an accidentally duplicated line of test code to fix bots.
Tue, Jan 21, 1:10 PM
aaron.ballman committed rG5260bc2497bb: Allow arbitrary capability name in Thread Safety Analysis (authored by eti-p-doray).
Allow arbitrary capability name in Thread Safety Analysis
Tue, Jan 21, 12:51 PM
aaron.ballman closed D72635: Allow arbitrary capability name in Thread Safety Analysis.

Thanks!
I don't I have commit access. Can you commit on my behalf?

Tue, Jan 21, 12:51 PM · Restricted Project
aaron.ballman accepted D73104: [Attr][Doc][NFC] Fix code snippet formatting for attribute documentation.

LGTM!

Tue, Jan 21, 10:30 AM · Restricted Project

Mon, Jan 20

aaron.ballman added inline comments to D71566: New checks for fortified sprintf.
Mon, Jan 20, 8:33 AM · Restricted Project
aaron.ballman added a comment to D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals..

Okay, but as you can see the majority of my test cases are intentionally false negatives - -Wsign-conversion triggers so often than many people don't use it.

Mon, Jan 20, 7:53 AM · Restricted Project, Restricted Project
aaron.ballman accepted D73007: [Sema] Avoid Wrange-loop-analysis false positives.

LGTM! Do you think this should also be pushed onto the release branch?

Mon, Jan 20, 7:44 AM · Restricted Project
aaron.ballman added a comment to D72488: [clang-tidy] Add check for CERT-OOP57-CPP.

Btw, do you need me to commit this on your behalf, or have you obtained your commit privileges?

Mon, Jan 20, 7:40 AM · Restricted Project, Restricted Project

Sat, Jan 18

aaron.ballman accepted D72635: Allow arbitrary capability name in Thread Safety Analysis.

LGTM with a minor testing request, thank you!

Sat, Jan 18, 6:35 AM · Restricted Project

Fri, Jan 17

aaron.ballman created D72948: Unconditionally enable lvalue function designators; NFC.
Fri, Jan 17, 12:47 PM · Restricted Project
aaron.ballman added a comment to rGbcda877b4309: Fix a compile error to get bots back to green..

Correct, it's not supported for MSVC yet. I just tried to enable it to see if MSVC 2019 has support and I get errors:

C:\llvm-project\llvm\include\llvm\ADT\Optional.h(290): error C2560: 'llvm::Optional<unknown-type> llvm::Optional<T>::map(const Function &) &&': cannot overload a member function with ref-qualifier with a member function without ref-qualifier

Perhaps the map overload just above that simply needs LLVM_LVALUE_FUNCTION on it? Like how the other getValueOr() and operator*s do.

Fri, Jan 17, 10:51 AM
aaron.ballman accepted D72484: [clang-tidy] Fix check for Abseil internal namespace access.

LGTM

Fri, Jan 17, 10:51 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to rGbcda877b4309: Fix a compile error to get bots back to green..

That's a bummer. I wrote *std::move(Fixup) intentionally that way, since llvm::Optional has a &&-qualified operator* overload that returns std::move of the contained value. I guess the Windows configuration doesn't have LLVM_HAS_RVALUE_REFERENCE_THIS?

Fri, Jan 17, 10:23 AM
aaron.ballman added a comment to D72875: [clang][cmake] Include generated rst files in html built by docs-clang-html target.

Thank you for working on this, I think this is a great improvement. I am not super qualified to review the cmake, but it looks eminently reasonable to me.

Fri, Jan 17, 7:46 AM · Restricted Project, Restricted Project
aaron.ballman committed rG7f4e744b90ec: Another speculative fix for the Windows bots. (authored by aaron.ballman).
Another speculative fix for the Windows bots.
Fri, Jan 17, 7:28 AM
aaron.ballman committed rGbcda877b4309: Fix a compile error to get bots back to green. (authored by aaron.ballman).
Fix a compile error to get bots back to green.
Fri, Jan 17, 6:59 AM
aaron.ballman added a comment to D72635: Allow arbitrary capability name in Thread Safety Analysis.

Hmm, I have been wondering about this as well. The way I see it, all of these things are what we call capabilities, and we treat them all the same. The names are just meant to make warning messages more readable, because what the analysis considers a capability, the user might know as a mutex, or role, or sequence.

Fri, Jan 17, 6:11 AM · Restricted Project
aaron.ballman closed D72378: [clang-tidy] Add `bugprone-reserved-identifier`.

Thank you for the new checker -- I've committed on your behalf in 42a0355816d3bc125d59cbd07052c8886e78ca86

Fri, Jan 17, 6:01 AM · Restricted Project, Restricted Project
aaron.ballman committed rG42a0355816d3: Add `bugprone-reserved-identifier` (authored by logan-5).
Add `bugprone-reserved-identifier`
Fri, Jan 17, 5:51 AM

Thu, Jan 16

aaron.ballman committed rGd5c6b8407c12: Factor out renaming logic from readability-identifier-naming (authored by logan-5).
Factor out renaming logic from readability-identifier-naming
Thu, Jan 16, 1:37 PM
aaron.ballman closed D72284: [clang-tidy] Factor out renaming logic from readability-identifier-naming.

Thank you for the patch, I've commit on your behalf in d5c6b8407c12d39a78f42a216369407cb2d7b511

Thu, Jan 16, 1:36 PM · Restricted Project, Restricted Project
aaron.ballman added a comment to D72378: [clang-tidy] Add `bugprone-reserved-identifier`.

Great! In that case, I'll need help committing this, as well as the thing it depends on, https://reviews.llvm.org/D72284 (which has also been LGTM'd).

Thu, Jan 16, 1:36 PM · Restricted Project, Restricted Project
aaron.ballman added reviewers for D72635: Allow arbitrary capability name in Thread Safety Analysis: aaronpuchert, delesley.

I added an example in the description.

From doc https://clang.llvm.org/docs/ThreadSafetyAnalysis.html,
it sounds like we should be allowed to declare our class with CAPABILITY("context"), but it turns out that only "mutex" and "role" are allowed.
I could otherwise update this CL to allow any string (single word lowercase?) as CAPABILITY?

Thu, Jan 16, 9:44 AM · Restricted Project
aaron.ballman added a comment to D71707: clang-tidy: new bugprone-pointer-cast-widening.

Have you considered language extensions like __ptr32, __ptr64, __sptr, and __uptr (https://docs.microsoft.com/en-us/cpp/cpp/ptr32-ptr64?view=vs-2019) which we also support?

I think only __uptr support would matter to prevent some false positives. I am not sure it is used anywhere. Should I really implement it?

Thu, Jan 16, 7:32 AM · Restricted Project, Restricted Project
aaron.ballman accepted D72488: [clang-tidy] Add check for CERT-OOP57-CPP.

LGTM with a few nits.

Thu, Jan 16, 7:11 AM · Restricted Project, Restricted Project
aaron.ballman accepted D72378: [clang-tidy] Add `bugprone-reserved-identifier`.

LGTM!

Thu, Jan 16, 7:01 AM · Restricted Project, Restricted Project
aaron.ballman requested changes to D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals..

Given that the compiler already has -Wsign-conversion, I'm not certain what value is added by this check. Can you explain a bit about why this is the correct approach for diagnosing the issue? When you ignore the false positives from the test, the only cases not pointed out by -Wsign-compare are the macro cases (which is reasonable behavior to not diagnose on -- the fix for one macro may make other macros invalid).

Thu, Jan 16, 6:51 AM · Restricted Project, Restricted Project
aaron.ballman added inline comments to D72484: [clang-tidy] Fix check for Abseil internal namespace access.
Thu, Jan 16, 6:42 AM · Restricted Project, Restricted Project
aaron.ballman accepted D72373: [clang-tidy] extend misc-misplaced-const to detect using besides typedef.

FIX-IT isn't quite that obvious. Some options:

  • look for other typedef which contains the same as const
  • create new typedef/using
  • remove "*" from typedef and adjust all usage accordingly. Implies removing "Ptr" suffix.
Thu, Jan 16, 6:23 AM · Restricted Project, Restricted Project

Wed, Jan 15

aaron.ballman added a comment to D72635: Allow arbitrary capability name in Thread Safety Analysis.

Can you give some examples of code where you think the diagnostics are inappropriate? One of the goals of role-based capabilities is for you to name the capabilities as you see fit, so it's possible that there is a different way for us to surface those diagnostics for your situation that doesn't involve adding another one-off name for capabilities.

Wed, Jan 15, 6:11 AM · Restricted Project
aaron.ballman accepted D72518: [clang] New __attribute__((arm_mve_strict_polymorphism))..

Aside from changing the attribute name back to what it was (sorry about that), this LGTM!

Wed, Jan 15, 6:02 AM · Restricted Project
aaron.ballman committed rGee0f1f1edc3e: Further implement CWG 2292 (authored by Manna).
Further implement CWG 2292
Wed, Jan 15, 5:52 AM
aaron.ballman closed D72219: Patch for Bug 43966 - Clang compiler fails with error: expected the class name after '~' to name a destructor.

Thank you for the patch, I've committed on your behalf in ee0f1f1edc3ec0d4e698d50cc3180217448802b7

Wed, Jan 15, 5:52 AM
aaron.ballman added a comment to D68115: Zero initialize padding in unions.

I would be happy to finish this patch if we agree on something.

So if I understand this the proposal is to have something like -fzero-union-padding which is off by default.
When it's OFF compiler will continue to do whatever it does now.
When it's ON it will set zeroes into padding with or without -ftrivial-auto-var-init.
Is this correct?

In general, I believe so, yes. To be clear, it only sets zeros into union padding, not *all* padding. I do not have an opinion on whether we want it to be -fzero-union-padding as opposed to -finit-union-padding that honors the pattern from -ftrivial-auto-init=pattern and defaults to zero if no pattern is specified.

They whole point of the patch was to avoid breaking code by -ftrivial-auto-init=pattern with "MyUnion my_union = {}". So to fix that only -fzero-union-padding behavior helpful.
-ftrivial-auto-init=pattern as-is already inits union padding with patterns.

Wed, Jan 15, 5:43 AM · Restricted Project

Tue, Jan 14

aaron.ballman added a comment to D68115: Zero initialize padding in unions.

I would be happy to finish this patch if we agree on something.

So if I understand this the proposal is to have something like -fzero-union-padding which is off by default.
When it's OFF compiler will continue to do whatever it does now.
When it's ON it will set zeroes into padding with or without -ftrivial-auto-var-init.
Is this correct?

Tue, Jan 14, 1:38 PM · Restricted Project
aaron.ballman committed rG36fcbb838c8f: Added readability-qualified-auto check (authored by njames93).
Added readability-qualified-auto check
Tue, Jan 14, 11:12 AM
aaron.ballman closed D72217: [clang-tidy] Added readability-qualified-auto check.

Thank you for the new check, I've commit on your behalf in 36fcbb838c8f293f46bfed78c6ed8c177f1e3485

Tue, Jan 14, 11:11 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D72217: [clang-tidy] Added readability-qualified-auto check.

Do you need someone to commit this on your behalf? (If you plan to continue contributing, which I hope you do, it may be a good time for you to obtain commit privileges: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access but I am happy to commit on your behalf if you'd prefer.)

For now I would appreciate it, I have fired off an email though, cheers.

Tue, Jan 14, 8:02 AM · Restricted Project, Restricted Project
aaron.ballman accepted D72691: [clang-tidy] Match InitListExpr in modernize-use-default-member-init.

LGTM!

Tue, Jan 14, 6:56 AM · Restricted Project
aaron.ballman accepted D72219: Patch for Bug 43966 - Clang compiler fails with error: expected the class name after '~' to name a destructor.

The changes LGTM, but I notice that our DR status page says we already supported CWG2292 as of Clang 9 (Richard changed the status in svn:368941) which surprises me. Was this aspect just missed with the original commit, or is something else going on?

Tue, Jan 14, 6:45 AM
aaron.ballman added a comment to D72378: [clang-tidy] Add `bugprone-reserved-identifier`.

I think this patch is getting pretty close to finished -- have you tried running it over a large corpus of code to see if it spots reserved identifiers (or unreserved ones)? If not, I would recommend running it over LLVM + clang + clang-tools-extra to see how it operates when looking for reserved identifiers and libc++ for unreserved ones.

Tue, Jan 14, 6:35 AM · Restricted Project, Restricted Project
aaron.ballman added inline comments to D72488: [clang-tidy] Add check for CERT-OOP57-CPP.
Tue, Jan 14, 6:26 AM · Restricted Project, Restricted Project
aaron.ballman added a comment to D72217: [clang-tidy] Added readability-qualified-auto check.

Do you need someone to commit this on your behalf? (If you plan to continue contributing, which I hope you do, it may be a good time for you to obtain commit privileges: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access but I am happy to commit on your behalf if you'd prefer.)

Tue, Jan 14, 5:56 AM · Restricted Project, Restricted Project
aaron.ballman added inline comments to D72518: [clang] New __attribute__((arm_mve_strict_polymorphism))..
Tue, Jan 14, 5:56 AM · Restricted Project
aaron.ballman added a comment to D68115: Zero initialize padding in unions.

Does this have to be an unilateral change,
likely penalizing non--ftrivial-auto-var-init= cases,
i.e. [why] can't it be only done for when -ftrivial-auto-var-init= is enabled?

We left off near that conclusion (https://reviews.llvm.org/D68115#1686887);

Would be great if @rsmith / @aaron.ballman could comment on that

Tue, Jan 14, 5:45 AM · Restricted Project