Page MenuHomePhabricator

steveire (Stephen Kelly)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 5 2013, 6:50 AM (368 w, 17 h)

Recent Activity

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: 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: 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: Ignore template instantiations if not in AsIs mode.

Update

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

Update

Jun 12 2020, 12:02 PM · Restricted Project
steveire added a comment to D80961: 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: Ignore template instantiations if not in AsIs mode.

Port unit tests

Jun 7 2020, 3:59 PM · Restricted Project
steveire retitled D80961: 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: 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: 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: 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: 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: 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: 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: 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: Ignore template instantiations if not in AsIs mode.

Update

Jun 1 2020, 3:43 PM · Restricted Project
steveire created D80961: 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

May 21 2020

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.

May 21 2020, 3:43 PM · Restricted Project, Restricted Project
steveire updated the diff for D72534: Change default traversal in AST Matchers to ignore invisible nodes.

Touch

May 21 2020, 3:43 PM · Restricted Project

May 18 2020

steveire added inline comments to D72531: Set traversal explicitly where needed in tests.
May 18 2020, 1:00 PM · Restricted Project

May 17 2020

steveire added inline comments to D72531: Set traversal explicitly where needed in tests.
May 17 2020, 4:15 AM · Restricted Project
steveire added a comment to D72530: Set traversal explicitly where needed in clang-tidy.

The changes here look reasonable, but can some of this code be simplified to use the default behavior and less complex matchers that don't have to carefully ignore implicit nodes? This would demonstrate that the feature really is an improvement over the status quo and would also exercise more test code with the new defaults demonstrating equivalence. I'm not envisioning anything onerous, more just wondering if you think there is some low-hanging fruit that could be picked rather than converting everything to AsIs.

May 17 2020, 3:43 AM · Restricted Project

May 12 2020

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

Update

May 12 2020, 4:09 PM · Restricted Project
steveire updated the diff for D72534: Change default traversal in AST Matchers to ignore invisible nodes.

Update

May 12 2020, 4:09 PM · Restricted Project
steveire updated the diff for D72532: Make the ExprMutationAnalyzer explicit about how it traverses the AST.

Update

May 12 2020, 3:37 PM · Restricted Project
steveire updated the diff for D72531: Set traversal explicitly where needed in tests.

Update

May 12 2020, 3:37 PM · Restricted Project
steveire updated the diff for D72530: Set traversal explicitly where needed in clang-tidy.

Update

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

Update

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

Update

May 12 2020, 3:37 PM · Restricted Project

Jan 28 2020

steveire updated the diff for D72531: Set traversal explicitly where needed in tests.

Rebase

Jan 28 2020, 4:59 AM · Restricted Project
steveire updated the diff for D72530: Set traversal explicitly where needed in clang-tidy.

Rebase

Jan 28 2020, 3:00 AM · Restricted Project

Jan 26 2020

steveire abandoned D73029: Extend ExprTraversal class with acending traversal.
Jan 26 2020, 11:18 AM · Restricted Project
steveire abandoned D73028: Extract ExprTraversal class from Expr.
Jan 26 2020, 11:18 AM · Restricted Project

Jan 25 2020

steveire added a comment to D73388: NFC: Implement AST node skipping in ParentMapContext.

This seems reasonable to me.

If you want to unify something about this and the Ignore* functions in Expr, maybe we could add a "classify" mechanism, so you could ask "what kind of node is this?" and get back an enumerated value that indicates whether it's semantic / syntactic / whatever. Then we could implement both this and the downward-skipping in terms of that.

Jan 25 2020, 3:18 AM · Restricted Project

Jan 24 2020

steveire retitled D73388: NFC: Implement AST node skipping in ParentMapContext from Implement AST node skipping in ParentMapContext to NFC: Implement AST node skipping in ParentMapContext.
Jan 24 2020, 4:18 PM · Restricted Project
steveire updated the diff for D73388: NFC: Implement AST node skipping in ParentMapContext.

constness

Jan 24 2020, 4:09 PM · Restricted Project
steveire 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.

Thanks for the heads-up - I'm trying to reach the conclusion from this information.

Are you saying that

  1. this patch shouldn't be committed, so down-traversal-skipping should remain in Expr.cpp
  2. D73029 should be changed to implement the up-traversal-skipping directly in ParentMapContext.cpp

Is that the right conclusion?

Jan 24 2020, 4:09 PM · Restricted Project
steveire created D73388: NFC: Implement AST node skipping in ParentMapContext.
Jan 24 2020, 4:09 PM · Restricted Project
steveire added a comment to D71313: [AST] Split parent map traversal logic into ParentMapContext.h.

Should this new class have some tests?

Jan 24 2020, 3:59 PM · Restricted Project, Restricted Project
steveire 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.

Jan 24 2020, 1:53 PM · Restricted Project
steveire added a comment to D73028: Extract ExprTraversal class from Expr.

An addition to the API will be concerned with ascending through the AST in different traversal modes.

Ascending through the AST is not possibly by design. (For example, we share AST nodes between template instantiations, and statement nodes don't contain parent pointers.) Can you say more about what you're thinking of adding here?

Jan 24 2020, 12:36 PM · Restricted Project
steveire updated the diff for D73028: Extract ExprTraversal class from Expr.

Update

Jan 24 2020, 12:36 PM · Restricted Project
steveire updated the diff for D73028: Extract ExprTraversal class from Expr.

Update for review comments

Jan 24 2020, 12:36 PM · Restricted Project

Jan 20 2020

steveire created D73037: Add a way to set traversal mode in clang-query.
Jan 20 2020, 5:40 AM · Restricted Project
steveire updated the diff for D73029: Extend ExprTraversal class with acending traversal.

Format

Jan 20 2020, 4:35 AM · Restricted Project
steveire updated the diff for D73028: Extract ExprTraversal class from Expr.

Format

Jan 20 2020, 4:35 AM · Restricted Project
steveire created D73029: Extend ExprTraversal class with acending traversal.
Jan 20 2020, 4:07 AM · Restricted Project
steveire created D73028: Extract ExprTraversal class from Expr.
Jan 20 2020, 4:07 AM · Restricted Project

Jan 10 2020

steveire updated the diff for D72531: Set traversal explicitly where needed in tests.

Fix case

Jan 10 2020, 12:42 PM · Restricted Project
steveire updated the diff for D72530: Set traversal explicitly where needed in clang-tidy.

Fix case

Jan 10 2020, 12:42 PM · Restricted Project
steveire added a comment to D72233: Add a new AST matcher 'optionally'..

LGTM too. I described a more ad-hoc implementation previously: https://steveire.wordpress.com/2018/11/20/composing-ast-matchers-in-clang-tidy/ but I prefer this implementation.

Jan 10 2020, 12:33 PM
steveire created D72534: Change default traversal in AST Matchers to ignore invisible nodes.
Jan 10 2020, 11:56 AM · Restricted Project
steveire created D72532: Make the ExprMutationAnalyzer explicit about how it traverses the AST.
Jan 10 2020, 11:55 AM · Restricted Project
steveire created D72531: Set traversal explicitly where needed in tests.
Jan 10 2020, 11:55 AM · Restricted Project
steveire created D72530: Set traversal explicitly where needed in clang-tidy.
Jan 10 2020, 11:55 AM · Restricted Project

Dec 29 2019

steveire created D71977: Implement additional traverse() overloads.
Dec 29 2019, 12:41 PM · Restricted Project
steveire created D71976: Match code following lambdas when ignoring invisible nodes.
Dec 29 2019, 12:41 PM · Restricted Project

Dec 26 2019

steveire added inline comments to D71842: Allow newlines in AST Matchers in clang-query files.
Dec 26 2019, 9:47 AM · Restricted Project
steveire updated the diff for D71842: Allow newlines in AST Matchers in clang-query files.

Update

Dec 26 2019, 9:10 AM · Restricted Project
steveire added inline comments to D71842: Allow newlines in AST Matchers in clang-query files.
Dec 26 2019, 9:10 AM · Restricted Project

Dec 23 2019

steveire created D71842: Allow newlines in AST Matchers in clang-query files.
Dec 23 2019, 9:06 AM · Restricted Project

Dec 18 2019

steveire updated the diff for D71680: Customize simplified dumping and matching of LambdaExpr.

Update

Dec 18 2019, 4:23 PM · Restricted Project
steveire updated the diff for D71680: Customize simplified dumping and matching of LambdaExpr.

Update

Dec 18 2019, 4:23 PM · Restricted Project
steveire created D71680: Customize simplified dumping and matching of LambdaExpr.
Dec 18 2019, 2:58 PM · Restricted Project

Dec 10 2019

steveire added a comment to D70613: Add method to ignore invisible AST nodes.

It wasn't possible to add the const because of the return type.

Dec 10 2019, 1:42 PM · Restricted Project
steveire updated the diff for D70613: Add method to ignore invisible AST nodes.

Update

Dec 10 2019, 1:42 PM · Restricted Project

Dec 7 2019

steveire created D71166: Deprecate the hasDefaultArgument matcher.
Dec 7 2019, 10:25 AM · Restricted Project

Dec 6 2019

steveire closed D54406: Add matchDynamic convenience functions.

Committed in 2e8dc8590d8b412

Dec 6 2019, 4:07 PM · Restricted Project
steveire closed D62056: Use ASTDumper to dump the AST from clang-query.

Committed in b22d8ae7f436bfe63. I don't know why this review didn't automatically update.

Dec 6 2019, 3:48 PM · Restricted Project
steveire added a comment to D61837: Make it possible control matcher traversal kind with ASTContext.

I tried using DenseMap, but couldn't make it compile. I stuck with std::map for now.

Dec 6 2019, 3:21 PM · Restricted Project

Nov 22 2019

steveire created D70613: Add method to ignore invisible AST nodes.
Nov 22 2019, 12:16 PM · Restricted Project
steveire updated the diff for D61837: Make it possible control matcher traversal kind with ASTContext.

Rebase and update

Nov 22 2019, 9:58 AM · Restricted Project

Oct 14 2019

steveire committed rL374845: Add myself to the github migration.
Add myself to the github migration
Oct 14 2019, 4:30 PM

May 19 2019

steveire committed rL361117: Add a Visit overload for DynTypedNode to ASTNodeTraverser.
Add a Visit overload for DynTypedNode to ASTNodeTraverser
May 19 2019, 6:05 AM
steveire committed rC361117: Add a Visit overload for DynTypedNode to ASTNodeTraverser.
Add a Visit overload for DynTypedNode to ASTNodeTraverser
May 19 2019, 6:05 AM