Page MenuHomePhabricator

steveire (Stephen Kelly)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 5 2013, 6:50 AM (377 w, 1 d)

Recent Activity

Sat, Nov 21

steveire requested review of D91918: Remove the IgnoreImplicitCastsAndParentheses traversal kind.
Sat, Nov 21, 10:55 AM · Restricted Project
steveire updated the diff for D91916: Remove automatic traversal from forEach matcher.

Update

Sat, Nov 21, 10:52 AM · Restricted Project
steveire requested review of D91917: Update mode used in traverse() examples.
Sat, Nov 21, 10:51 AM · Restricted Project
steveire requested review of D91916: Remove automatic traversal from forEach matcher.
Sat, Nov 21, 10:43 AM · Restricted Project

Fri, Nov 20

steveire updated the diff for D91639: Add documentation illustrating use of IgnoreUnlessSpelledInSource.

Update

Fri, Nov 20, 2:35 AM · Restricted Project
steveire added inline comments to D91639: Add documentation illustrating use of IgnoreUnlessSpelledInSource.
Fri, Nov 20, 2:35 AM · Restricted Project

Thu, Nov 19

steveire added inline comments to D91639: Add documentation illustrating use of IgnoreUnlessSpelledInSource.
Thu, Nov 19, 11:45 AM · Restricted Project
steveire updated the diff for D91639: Add documentation illustrating use of IgnoreUnlessSpelledInSource.

Update

Thu, Nov 19, 11:42 AM · Restricted Project

Wed, Nov 18

steveire requested changes to D91485: [clang-tidy] ElseAfterReturn check wont suggest fixes if preprocessor branches are involved.
Wed, Nov 18, 7:38 AM · Restricted Project
steveire added a comment to D90982: Ignore implicit nodes in IgnoreUnlessSpelledInSource mode.

I've removed the offending test (which in this case is the correct fix - it is not a temporary fix).

Wed, Nov 18, 4:45 AM · Restricted Project

Tue, Nov 17

steveire updated the diff for D91639: Add documentation illustrating use of IgnoreUnlessSpelledInSource.

Update

Tue, Nov 17, 11:26 AM · Restricted Project
steveire requested review of D91639: Add documentation illustrating use of IgnoreUnlessSpelledInSource.
Tue, Nov 17, 9:08 AM · Restricted Project
steveire added inline comments to D90984: Update matchers to be traverse-aware.
Tue, Nov 17, 6:55 AM · Restricted Project
steveire updated the diff for D90984: Update matchers to be traverse-aware.

Update

Tue, Nov 17, 6:54 AM · Restricted Project
steveire added a comment to D80499: Remove obsolete ignore*() matcher uses.

! In D80499#2353187, @alexfh wrote:
You should be ready for back and forth with this change, if users hit widespread issues not caught by tests. Maybe splitting it into separate pieces and involving check authors where practical may be reasonable.

Ok, I'll do that when https://reviews.llvm.org/D80961 and https://reviews.llvm.org/D82278 are merged.

Tue, Nov 17, 6:50 AM · Restricted Project
steveire updated the diff for D90984: Update matchers to be traverse-aware.

Update

Tue, Nov 17, 6:24 AM · Restricted Project
steveire updated the diff for D90982: Ignore implicit nodes in IgnoreUnlessSpelledInSource mode.

Update

Tue, Nov 17, 6:17 AM · Restricted Project

Mon, Nov 16

steveire added inline comments to D90984: Update matchers to be traverse-aware.
Mon, Nov 16, 7:41 AM · Restricted Project

Wed, Nov 11

steveire requested review of D91303: Simplify implementation of container-size-empty.
Wed, Nov 11, 3:14 PM · Restricted Project
steveire requested review of D91302: Handle template instantiations better in clang-tidy check.
Wed, Nov 11, 3:12 PM · Restricted Project

Tue, Nov 10

steveire added inline comments to D90984: Update matchers to be traverse-aware.
Tue, Nov 10, 12:23 PM · Restricted Project
steveire updated the diff for D90984: Update matchers to be traverse-aware.

Update

Tue, Nov 10, 12:20 PM · Restricted Project
steveire added inline comments to D91144: Add utility for testing if we're matching nodes AsIs.
Tue, Nov 10, 11:25 AM · Restricted Project
steveire added a comment to D90984: Update matchers to be traverse-aware.

I added the release note to D90982, which this change follows.

Tue, Nov 10, 1:52 AM · Restricted Project
steveire updated the diff for D90984: Update matchers to be traverse-aware.

Update

Tue, Nov 10, 1:48 AM · Restricted Project
steveire updated the diff for D90982: Ignore implicit nodes in IgnoreUnlessSpelledInSource mode.

Update

Tue, Nov 10, 1:47 AM · Restricted Project
steveire requested review of D91144: Add utility for testing if we're matching nodes AsIs.
Tue, Nov 10, 1:47 AM · Restricted Project

Sun, Nov 8

steveire updated the diff for D90982: Ignore implicit nodes in IgnoreUnlessSpelledInSource mode.

Update

Sun, Nov 8, 10:27 AM · Restricted Project
steveire added inline comments to D90392: [clang-tidy] Omit std::make_unique/make_shared for default initialization..
Sun, Nov 8, 9:50 AM · Restricted Project, Restricted Project

Sat, Nov 7

steveire updated the diff for D90982: Ignore implicit nodes in IgnoreUnlessSpelledInSource mode.

Update

Sat, Nov 7, 4:35 PM · Restricted Project
steveire updated the diff for D90982: Ignore implicit nodes in IgnoreUnlessSpelledInSource mode.

Update

Sat, Nov 7, 4:13 PM · Restricted Project
steveire added inline comments to D90392: [clang-tidy] Omit std::make_unique/make_shared for default initialization..
Sat, Nov 7, 2:11 PM · Restricted Project, Restricted Project

Fri, Nov 6

steveire requested review of D90984: Update matchers to be traverse-aware.
Fri, Nov 6, 3:45 PM · Restricted Project
steveire requested review of D90983: Change algorithms to return iterators.
Fri, Nov 6, 3:42 PM · Restricted Project
steveire requested review of D90982: Ignore implicit nodes in IgnoreUnlessSpelledInSource mode.
Fri, Nov 6, 3:42 PM · Restricted Project
steveire added inline comments to D90767: Add new matchers for dependent names in templates.
Fri, Nov 6, 8:24 AM · Restricted Project
steveire updated the diff for D90767: Add new matchers for dependent names in templates.

Update

Fri, Nov 6, 8:02 AM · Restricted Project

Thu, Nov 5

steveire added inline comments to D90767: Add new matchers for dependent names in templates.
Thu, Nov 5, 3:14 PM · Restricted Project
steveire updated the diff for D90767: Add new matchers for dependent names in templates.

Update

Thu, Nov 5, 3:13 PM · Restricted Project
steveire added inline comments to D90763: Traverse-ignore explicit template instantiations.
Thu, Nov 5, 3:11 PM · Restricted Project
steveire added inline comments to D90763: Traverse-ignore explicit template instantiations.
Thu, Nov 5, 2:51 PM · Restricted Project
steveire updated the diff for D90763: Traverse-ignore explicit template instantiations.

Uppate

Thu, Nov 5, 2:47 PM · Restricted Project

Wed, Nov 4

steveire updated the diff for D90767: Add new matchers for dependent names in templates.

Update

Wed, Nov 4, 8:14 AM · Restricted Project
steveire requested review of D90767: Add new matchers for dependent names in templates.
Wed, Nov 4, 8:11 AM · Restricted Project
steveire requested review of D90763: Traverse-ignore explicit template instantiations.
Wed, Nov 4, 7:07 AM · Restricted Project

Sun, Nov 1

steveire requested review of D90553: Rename CXXUnresolvedConstructExpr::arg_size for consistency.
Sun, Nov 1, 5:41 AM · Restricted Project

Thu, Oct 29

steveire added a comment to D80623: WIP: Add an API to simplify setting TraversalKind in clang-tidy matchers.

Reviving this so it can be used to port clang-tidy checks to IgnoreUnlessSpelledInSource.

Thu, Oct 29, 10:00 AM · Restricted Project
steveire updated the diff for D80623: WIP: Add an API to simplify setting TraversalKind in clang-tidy matchers.

Update

Thu, Oct 29, 9:58 AM · Restricted Project
steveire updated the diff for D82278: Fix traversal over CXXConstructExpr in Syntactic mode.

Update

Thu, Oct 29, 8:40 AM · Restricted Project
steveire added a comment to D80499: Remove obsolete ignore*() matcher uses.

! In D80499#2353187, @alexfh wrote:
You should be ready for back and forth with this change, if users hit widespread issues not caught by tests. Maybe splitting it into separate pieces and involving check authors where practical may be reasonable.

Thu, Oct 29, 7:42 AM · Restricted Project

Oct 26 2020

steveire added a comment to D80961: [clang][AST] Ignore template instantiations if not in AsIs mode.

Many of the changes which were part of a previous iteration of this change were related to the change of default behavior of matchers. As the default is no longer changed, those changes fell away.

Oct 26 2020, 6:19 AM · Restricted Project
steveire updated the diff for D80961: [clang][AST] Ignore template instantiations if not in AsIs mode.

Update

Oct 26 2020, 4:10 AM · Restricted Project
steveire updated the diff for D80961: [clang][AST] Ignore template instantiations if not in AsIs mode.

Rebase

Oct 26 2020, 4:06 AM · Restricted Project

Oct 25 2020

steveire added inline comments to D82278: Fix traversal over CXXConstructExpr in Syntactic mode.
Oct 25 2020, 12:06 PM · Restricted Project
steveire updated the diff for D82278: Fix traversal over CXXConstructExpr in Syntactic mode.

Rebased

Oct 25 2020, 12:02 PM · Restricted Project
steveire updated subscribers of D80499: Remove obsolete ignore*() matcher uses.

@alexfh This change is based on the behavior of AST Matchers being changed to ignore invisible/implicit AST nodes by default. As the default was not changed in the end, this patch would need to be updated to add traverse(TK_IgnoreUnlessSpelledInSource) wrapping around the matchers.

Oct 25 2020, 6:05 AM · Restricted Project

Oct 8 2020

steveire added a comment to D88275: [ASTMatchers] Add matcher `hasParentIgnoringImplicit`..

TL;DR Stephen's fix works; I'll drop this patch.

Oct 8 2020, 2:15 PM · Restricted Project

Oct 7 2020

steveire added a comment to D88275: [ASTMatchers] Add matcher `hasParentIgnoringImplicit`..

I don't get email notifications when I'm pinged on Phab for some reason. I didn't know about this until the email from Aaron.

Oct 7 2020, 4:29 PM · Restricted Project

Jul 4 2020

steveire added a reviewer for D82278: Fix traversal over CXXConstructExpr in Syntactic mode: aaron.ballman.
Jul 4 2020, 6:26 AM · Restricted Project

Jul 2 2020

steveire updated the diff for D83076: Revert AST Matchers default to AsIs mode.

Update

Jul 2 2020, 3:41 PM · Restricted Project
steveire updated the diff for D83076: Revert AST Matchers default to AsIs mode.

Update

Jul 2 2020, 3:08 PM · Restricted Project
steveire added a reviewer for D83076: Revert AST Matchers default to AsIs mode: sammccall.
Jul 2 2020, 12:57 PM · Restricted Project
steveire created D83076: Revert AST Matchers default to AsIs mode.
Jul 2 2020, 12:57 PM · Restricted Project

Jun 22 2020

steveire accepted D82279: Handle invalid types in the nullPointerConstant AST matcher.
Jun 22 2020, 3:35 PM

Jun 21 2020

steveire added inline comments to D82279: Handle invalid types in the nullPointerConstant AST matcher.
Jun 21 2020, 2:18 PM
steveire created D82278: Fix traversal over CXXConstructExpr in Syntactic mode.
Jun 21 2020, 7:25 AM · Restricted Project
steveire added a comment to D80961: [clang][AST] Ignore template instantiations if not in AsIs mode.

Here's another example where it is not appropriate to transform a template instantiation:

Jun 21 2020, 4:45 AM · Restricted Project

Jun 17 2020

steveire added a comment to D80961: [clang][AST] Ignore template instantiations if not in AsIs mode.
  1. the scare quotes around "standing objections" reads like you're not respecting the opinions of others here;
Jun 17 2020, 2:02 PM · Restricted Project

Jun 13 2020

steveire added a comment to D69764: [clang-format] Add East/West Const fixer capability.

@steveire Thanks for the additional test cases, I'm reworking how I handle the Macro case as I realized that I didn't actually even try to change the case @rsmith came up with in the first place. I made a decision previous that I couldn't handle any case without the *,& or && to determine that wasn't the case

Just ignoring upper case isn't a good idea either because of LONG,HRESULT and LRESULT types.

it may take me a couple of days, but I agree to have something akin to FOREACH macros might be the way to go.

Jun 13 2020, 5:37 PM · Restricted Project, Restricted Project

Jun 12 2020

steveire updated the diff for D80961: [clang][AST] Ignore template instantiations if not in AsIs mode.

Update

Jun 12 2020, 2:49 PM · Restricted Project
steveire updated the diff for D80961: [clang][AST] Ignore template instantiations if not in AsIs mode.

Update

Jun 12 2020, 12:02 PM · Restricted Project
steveire added a comment to D80961: [clang][AST] Ignore template instantiations if not in AsIs mode.

Without jumping into the discussion whether it should be the default, I think we should be able to control template instantiation visitation separately from other implicit nodes.
Having to put AsIs on a matcher every time you need to match template instantiations is a rather big change (suddenly you have to change all the matchers you've written so far).

I think that's the intended meaning of AsIs though. Template instantiations are not source code the user wrote, they're source code the compiler stamped out from code the user wrote. I hope IgnoreUnlessSpelledInSource isn't a misnomer.

I love the idea of being able to control visitation of template instantiation.
I am somewhat torn on whether it should be the default, and would like to see more data.
I feel more strongly about needing AsIs when I want to match template instantiations.

FWIW, my experience in clang-tidy has been that template instantiations are ignored far more often than they're desired. In fact, instantiations tend to be a source of bugs for us because they're easy to forget about when writing matchers without keeping templates in mind. The times when template instantiations become important to *not* ignore within the checks is when the check is specific to template behavior, but that's a minority of the public checks thus far.

So I assume the idea is that this will work & be what we'll want people to write?
traverse(AsIs, decl(traverse(IgnoreImplicit, hasFoo)))

I believe so, yes. It's explicit about which traversal mode you want any given set of matchers to match with, so it is more flexible at the expense of being more verbose. Alternatively, you could be in AsIs mode for all of it and explicitly ignore the implicit nodes you wish to ignore (which may be even more verbose with the same results, depending on the matchers involved).

Jun 12 2020, 10:52 AM · Restricted Project

Jun 7 2020

steveire updated the diff for D80961: [clang][AST] Ignore template instantiations if not in AsIs mode.

Port unit tests

Jun 7 2020, 3:59 PM · Restricted Project
steveire retitled D80961: [clang][AST] Ignore template instantiations if not in AsIs mode from WIP: Ignore template instantiations if not in AsIs mode to Ignore template instantiations if not in AsIs mode.
Jun 7 2020, 3:59 PM · Restricted Project
steveire added a comment to D81336: [clang-tidy] simplify-bool-expr ignores template instantiations.

This isn't needed if https://reviews.llvm.org/D80961 is merged.

Jun 7 2020, 3:59 PM · Restricted Project
steveire added a comment to D80961: [clang][AST] Ignore template instantiations if not in AsIs mode.

I don't think that's true. You have to change the matchers you've written which deliberately match the instantiation, but you can also (optionally) simplify the others to remove unless(isInTemplateInstantiation()) from them. The third category of matchers are the ones where you haven't used unless(isInTemplateInstantiation()) even though you should have and you have a bug that you don't know about yet. This change fixes those ones.

Jun 7 2020, 3:59 PM · Restricted Project

Jun 4 2020

steveire added a comment to D80961: [clang][AST] Ignore template instantiations if not in AsIs mode.

Hmm, IgnoreUnlessSpelledInSource is designed and named to fill that role.

Jun 4 2020, 4:03 PM · Restricted Project
steveire added a comment to D80961: [clang][AST] Ignore template instantiations if not in AsIs mode.

Without jumping into the discussion whether it should be the default, I think we should be able to control template instantiation visitation separately from other implicit nodes.

Jun 4 2020, 4:03 PM · Restricted Project

Jun 3 2020

steveire updated the summary of D80961: [clang][AST] Ignore template instantiations if not in AsIs mode.
Jun 3 2020, 4:35 PM · Restricted Project

Jun 2 2020

steveire added a comment to D80961: [clang][AST] Ignore template instantiations if not in AsIs mode.

If IgnoreUnlessSpelledInSource is indeed for novice users (and not to be strictly interpreted as "it does what it says") we should think about whether it more useful to ignore instantiations or to match in instantiations.

Jun 2 2020, 2:50 PM · Restricted Project
steveire added a comment to D80961: [clang][AST] Ignore template instantiations if not in AsIs mode.

Thank you for bringing up this issue. I think it highlights an underlying problem -- editing templates is quite difficult -- that neither setting will address, as Dmitri expanded on above. Given the parallel to macros, I'd say your change is better than the status quo. Most clang tidies and other rewriting tools that I've encountered simply skip code in macro expansions, rather than reason about how to update the macro definition or whatnot. So, by that reasoning, we should skip template instantations.

Jun 2 2020, 2:50 PM · Restricted Project
steveire added a comment to D80961: [clang][AST] Ignore template instantiations if not in AsIs mode.

My experience with clang-tidy has been that template instantiations are a double-edged sword. The instantiation is the only place at which you have sufficient information to perform many kinds of analyses, but it's also often not plausible to modify the templated code because it's not clear from the instantiation context how changes may impact other instantiations. The result is that most of the clang-tidy checks wind up punting on template instantiations similar to what they do for macros. Based on that experience, I think it makes sense to continue in the proposed direction.

Jun 2 2020, 2:50 PM · Restricted Project

Jun 1 2020

steveire updated the diff for D80961: [clang][AST] Ignore template instantiations if not in AsIs mode.

Update

Jun 1 2020, 3:43 PM · Restricted Project
steveire created D80961: [clang][AST] Ignore template instantiations if not in AsIs mode.
Jun 1 2020, 3:43 PM · Restricted Project

May 29 2020

steveire added a comment to D69764: [clang-format] Add East/West Const fixer capability.

Here's some more failing testcases.

May 29 2020, 11:27 AM · Restricted Project, Restricted Project

May 27 2020

steveire added inline comments to D80654: WIP: Make it possible to use the traverse() matcher in clang-query.
May 27 2020, 2:42 PM · Restricted Project
steveire added a comment to D80654: WIP: Make it possible to use the traverse() matcher in clang-query.

This is not ready for review, but is it a direction we want to go, given that we can already use

May 27 2020, 1:02 PM · Restricted Project
steveire created D80654: WIP: Make it possible to use the traverse() matcher in clang-query.
May 27 2020, 12:29 PM · Restricted Project
steveire added a comment to D80606: [libTooling] Fix Transformer to work with ambient traversal kinds.

It might make sense for now (in order to unblock you) to make the Transformer library explicitly require the AsIs mode. I am not so familiar with the transformer library, but I think you can do that by adding traverse(AsIs, ...) in Transformer::registerMatchers.

May 27 2020, 5:55 AM · Restricted Project
steveire created D80623: WIP: Add an API to simplify setting TraversalKind in clang-tidy matchers.
May 27 2020, 5:55 AM · Restricted Project

May 26 2020

steveire added a comment to D54408: [ASTMatchers] Add matchers available through casting to derived.

@aaron.ballman I think we agreed in Belfast in November (after the most recent comment) to get this in as it is and not be as draconian about auto. Is anything blocking this?

May 26 2020, 5:28 PM · Restricted Project
steveire added a comment to D69764: [clang-format] Add East/West Const fixer capability.

I'm uncomfortable about clang-format performing this transformation at all. Generally, clang-format only makes changes that are guaranteed to preserve the meaning of the source program, and does not make changes that are only heuristically likely to be semantics-preserving. But this transformation is not behavior-preserving in a variety of cases, such as:

#define INTPTR int *
const INTPTR a;
// INTPTR const a; means something else
May 26 2020, 4:55 PM · Restricted Project, Restricted Project
steveire added inline comments to D69764: [clang-format] Add East/West Const fixer capability.
May 26 2020, 2:12 PM · Restricted Project, Restricted Project
steveire added a comment to D69764: [clang-format] Add East/West Const fixer capability.

if I put any declarations inside the preprocess clauses they actually don't get converted.

May 26 2020, 1:38 PM · Restricted Project, Restricted Project
steveire added a comment to D69764: [clang-format] Add East/West Const fixer capability.

@MyDeveloperDay Thanks for the update. I pinged you on slack about this, but I guess you're not using it at the moment. I asked if you have a git branch somewhere with this change. Downloading patches from phab is such a pain I have no idea why we use it.

May 26 2020, 12:32 PM · Restricted Project, Restricted Project

May 24 2020

steveire created D80499: Remove obsolete ignore*() matcher uses.
May 24 2020, 4:31 PM · Restricted Project
steveire updated the diff for D72534: Change default traversal in AST Matchers to ignore invisible nodes.

Rebase

May 24 2020, 2:55 PM · Restricted Project

May 22 2020

steveire updated the diff for D72534: Change default traversal in AST Matchers to ignore invisible nodes.

Touch

May 22 2020, 6:14 PM · Restricted Project
steveire updated the diff for D73037: Add a way to set traversal mode in clang-query.

Update

May 22 2020, 6:14 PM · Restricted Project
steveire added a comment to D69764: [clang-format] Add East/West Const fixer capability.

I like the approach of using clang-format to implement this. It's much faster than a clang-tidy approach.

The broader C++ community has already chosen East/West and it has momentum. If you choose Left/Right now, you will get pressure to add East/West in the future, which means we'll have the synonyms we want to avoid.

The broader C++ community already has understanding of East/West. Trying to change that now should be out of scope for this patch. This patch should use East/West.

I ran this on a large codebase and discovered some problems with this patch. Given this .clang-format file:

Thank you for this feedback @steveire, to be honest I agree, I didn't want to waste time arguing about the naming for now so simply gave in. Supporting multiple words from the outset also felt wrong, maybe we can spin around later towards the end of the review if there is more of a concencus on naming being the other way.

May 22 2020, 3:02 PM · Restricted Project, Restricted Project