Page MenuHomePhabricator

ymandel (Yitzhak Mandelbaum)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 27 2018, 12:45 PM (107 w, 4 d)

Recent Activity

Thu, Oct 15

ymandel committed rG65cb4fdd69f4: [libTooling] Change `after` range-selector to operate only on source ranges (authored by ymandel).
[libTooling] Change `after` range-selector to operate only on source ranges
Thu, Oct 15, 1:59 PM
ymandel closed D89468: [libTooling] Change `after` range-selector to operate only on source ranges.
Thu, Oct 15, 1:59 PM · Restricted Project
ymandel added inline comments to D89468: [libTooling] Change `after` range-selector to operate only on source ranges.
Thu, Oct 15, 8:33 AM · Restricted Project
ymandel updated the diff for D89468: [libTooling] Change `after` range-selector to operate only on source ranges.

cleaned up test code

Thu, Oct 15, 8:30 AM · Restricted Project
ymandel requested review of D89468: [libTooling] Change `after` range-selector to operate only on source ranges.
Thu, Oct 15, 7:34 AM · Restricted Project

Thu, Oct 8

ymandel abandoned D88275: [ASTMatchers] Add matcher `hasParentIgnoringImplicit`..
Thu, Oct 8, 6:37 AM · Restricted Project
ymandel added a comment to D88275: [ASTMatchers] Add matcher `hasParentIgnoringImplicit`..

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

Thu, Oct 8, 6:37 AM · Restricted Project

Wed, Oct 7

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

Thank you for your patience while we sort this out. I've pinged @steveire off-list, so hopefully he'll respond with his thoughts. If we don't hear from Stephen in the next week or so, I think we should proceed with your proposed patch to get you unblocked (adding one more "ignore implicit" variant isn't the end of the world, I just want us to be thoughtful about whether we want to add new matchers in this space or to work on the traversal mode instead).

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

Mon, Oct 5

ymandel 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.

Mon, Oct 5, 7:29 PM · Restricted Project

Wed, Sep 30

ymandel 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.

Wed, Sep 30, 6:42 AM · Restricted Project

Fri, Sep 25

ymandel updated the diff for D88275: [ASTMatchers] Add matcher `hasParentIgnoringImplicit`..

restored changes to unrelated parts of html docs.

Fri, Sep 25, 10:47 AM · Restricted Project
ymandel added a comment to D88275: [ASTMatchers] Add matcher `hasParentIgnoringImplicit`..

This seems to be strongly related to the TK_IgnoreUnlessSpelledInSource traversal behavior; is there a reason you can't use that traversal mode with hasParent() (does it behave differently)?

Fri, Sep 25, 10:40 AM · Restricted Project
ymandel requested review of D88319: [AST] Delete broken, unused template..
Fri, Sep 25, 9:43 AM · Restricted Project
ymandel updated the diff for D88275: [ASTMatchers] Add matcher `hasParentIgnoringImplicit`..

Fixed to use more standard type adaptors. Registration now works.

Fri, Sep 25, 9:33 AM · Restricted Project
ymandel updated the diff for D88275: [ASTMatchers] Add matcher `hasParentIgnoringImplicit`..

update dynamic registry and the ast matcher doc.

Fri, Sep 25, 7:51 AM · Restricted Project

Thu, Sep 24

ymandel requested review of D88275: [ASTMatchers] Add matcher `hasParentIgnoringImplicit`..
Thu, Sep 24, 8:04 PM · Restricted Project

Sep 15 2020

ymandel accepted D87527: [ASTMatchers] Fix `hasBody` for the descendants of `FunctionDecl`.

Thanks, this looks great! But, can you also please update https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/modernize/UseEqualsDeleteCheck.cpp#L39, since it depends on the current semantics?

Sep 15 2020, 9:44 AM · Restricted Project

Sep 14 2020

ymandel added a comment to D87527: [ASTMatchers] Fix `hasBody` for the descendants of `FunctionDecl`.

We must decide about the namings. If we want to be in sync with the methods in FunctionDecl, then we keep hasBody() as is, but remove the template specialization, and create a new doesThisDeclarationHaveABody(). However, this involves changing existing checks. The other solution is to fix hasBody() like in this patch, and find a new name for the other behavior, such as e.g. hasBodySomewhere() or something similar.

Sep 14 2020, 7:47 AM · Restricted Project
ymandel added a comment to D87527: [ASTMatchers] Fix `hasBody` for the descendants of `FunctionDecl`.

Can you expand on what is wrong currently for FunctionDecl descendants? Would the new test FindsBodyOfFunctionChildren fail with the current implementation?

Yes, exactly. They return 2 findings instead of 1 because the matcher matches both the forward declaration and the definition. The cause for this is that template specializations do not work for the descendants of the class for which is template specialized. Instead, the generic version is used. Thus for the children of FunctionDecl the original GetBodyMatcher is instantiated which does not check whether doesThisDeclarationHaveABody() but returns a body instead which may be bound to another declaration of the same function in the declaration chain. This is obviously wrong.

Sep 14 2020, 6:10 AM · Restricted Project
ymandel added a comment to D87527: [ASTMatchers] Fix `hasBody` for the descendants of `FunctionDecl`.

Can you expand on what is wrong currently for FunctionDecl descendants? Would the new test FindsBodyOfFunctionChildren fail with the current implementation?

Sep 14 2020, 5:13 AM · Restricted Project

Sep 11 2020

ymandel committed rGa5cefd95cc60: [libTooling] Fix use of `char` in comparison. (authored by ymandel).
[libTooling] Fix use of `char` in comparison.
Sep 11 2020, 5:25 AM
ymandel closed D87409: [libTooling] Fix use of `char` in comparison..
Sep 11 2020, 5:24 AM · Restricted Project

Sep 10 2020

ymandel added a reviewer for D87409: [libTooling] Fix use of `char` in comparison.: gribozavr.
Sep 10 2020, 8:11 AM · Restricted Project

Sep 9 2020

ymandel requested review of D87409: [libTooling] Fix use of `char` in comparison..
Sep 9 2020, 12:14 PM · Restricted Project

Sep 3 2020

ymandel committed rGd4f390313129: [libTooling] Provide overloads of `rewriteDescendants` that operate directly on… (authored by ymandel).
[libTooling] Provide overloads of `rewriteDescendants` that operate directly on…
Sep 3 2020, 7:41 AM
ymandel closed D87031: [libTooling] Provide overloads of `rewriteDescendants` that operate directly on an AST node..
Sep 3 2020, 7:41 AM · Restricted Project

Sep 2 2020

ymandel added a comment to D87031: [libTooling] Provide overloads of `rewriteDescendants` that operate directly on an AST node..

Thanks for the review!

Sep 2 2020, 7:43 PM · Restricted Project
ymandel updated subscribers of D87031: [libTooling] Provide overloads of `rewriteDescendants` that operate directly on an AST node..
Sep 2 2020, 7:41 PM · Restricted Project
ymandel removed reviewers for D87031: [libTooling] Provide overloads of `rewriteDescendants` that operate directly on an AST node.: jdoerfert, jhenderson, nicolasvasilache, rriddle, DavidTruby, aartbik, Restricted Project.
Sep 2 2020, 7:39 PM · Restricted Project
ymandel updated the diff for D87031: [libTooling] Provide overloads of `rewriteDescendants` that operate directly on an AST node..

fix diff base; make small clang-tidy suggested change

Sep 2 2020, 7:36 PM · Restricted Project
ymandel updated the diff for D87031: [libTooling] Provide overloads of `rewriteDescendants` that operate directly on an AST node..

Moved new rewriteDescendants overloads to detail namespace

Sep 2 2020, 7:34 PM · Restricted Project
ymandel committed rG6f0a3711bc15: [libTooling] Restore defaults for matchers in makeRule. (authored by ymandel).
[libTooling] Restore defaults for matchers in makeRule.
Sep 2 2020, 12:37 PM
ymandel closed D87048: [libTooling] Restore defaults for matchers in makeRule..
Sep 2 2020, 12:37 PM · Restricted Project
ymandel requested review of D87048: [libTooling] Restore defaults for matchers in makeRule..
Sep 2 2020, 11:48 AM · Restricted Project
ymandel requested review of D87031: [libTooling] Provide overloads of `rewriteDescendants` that operate directly on an AST node..
Sep 2 2020, 7:44 AM · Restricted Project

Aug 11 2020

ymandel committed rGd8c1f43dcc94: [libTooling] Move RewriteRule include edits to ASTEdit granularity. (authored by ymandel).
[libTooling] Move RewriteRule include edits to ASTEdit granularity.
Aug 11 2020, 9:48 AM
ymandel closed D85734: [libTooling] Move RewriteRule include edits to ASTEdit granularity..
Aug 11 2020, 9:48 AM · Restricted Project
ymandel committed rG645dd1b3bf8d: [libTooling] Cleanup and reorder `RewriteRule.h`. (authored by ymandel).
[libTooling] Cleanup and reorder `RewriteRule.h`.
Aug 11 2020, 9:36 AM
ymandel closed D85733: [libTooling] Cleanup and reorder `RewriteRule.h`..
Aug 11 2020, 9:36 AM · Restricted Project
ymandel updated the diff for D85734: [libTooling] Move RewriteRule include edits to ASTEdit granularity..

reword comment per suggestion.

Aug 11 2020, 9:35 AM · Restricted Project
ymandel updated the diff for D85733: [libTooling] Cleanup and reorder `RewriteRule.h`..

fixed typo

Aug 11 2020, 9:25 AM · Restricted Project
ymandel updated the diff for D85734: [libTooling] Move RewriteRule include edits to ASTEdit granularity..

fixed typo

Aug 11 2020, 9:20 AM · Restricted Project
ymandel updated the diff for D85734: [libTooling] Move RewriteRule include edits to ASTEdit granularity..

Updated clang-tidy transformer interpreter correspondingly.

Aug 11 2020, 9:18 AM · Restricted Project
ymandel updated the diff for D85734: [libTooling] Move RewriteRule include edits to ASTEdit granularity..

tweak

Aug 11 2020, 7:42 AM · Restricted Project
ymandel requested review of D85734: [libTooling] Move RewriteRule include edits to ASTEdit granularity..
Aug 11 2020, 7:40 AM · Restricted Project
ymandel updated the diff for D85733: [libTooling] Cleanup and reorder `RewriteRule.h`..

fix typo

Aug 11 2020, 7:32 AM · Restricted Project
ymandel requested review of D85733: [libTooling] Cleanup and reorder `RewriteRule.h`..
Aug 11 2020, 7:25 AM · Restricted Project

Jul 28 2020

ymandel committed rG04a21318b557: [libTooling] Add a `between` range-selector combinator. (authored by ymandel).
[libTooling] Add a `between` range-selector combinator.
Jul 28 2020, 10:26 AM
ymandel closed D84315: [libTooling] Add a `between` range-selector combinator..
Jul 28 2020, 10:26 AM · Restricted Project
ymandel updated the diff for D84315: [libTooling] Add a `between` range-selector combinator..

updated comment

Jul 28 2020, 9:54 AM · Restricted Project

Jul 24 2020

ymandel committed rGc332a984aefc: [libTooling] Add an `EditGenerator` that applies a rule throughout a bound node. (authored by ymandel).
[libTooling] Add an `EditGenerator` that applies a rule throughout a bound node.
Jul 24 2020, 7:39 AM
ymandel closed D84409: [libTooling] Add an `EditGenerator` that applies a rule throughout a bound node..
Jul 24 2020, 7:38 AM · Restricted Project
ymandel added inline comments to D84409: [libTooling] Add an `EditGenerator` that applies a rule throughout a bound node..
Jul 24 2020, 7:34 AM · Restricted Project
ymandel updated the diff for D84409: [libTooling] Add an `EditGenerator` that applies a rule throughout a bound node..

addressed comments.

Jul 24 2020, 7:21 AM · Restricted Project
ymandel committed rGcf428778128f: [libTooling] Add assorted `EditGenerator` combinators. (authored by ymandel).
[libTooling] Add assorted `EditGenerator` combinators.
Jul 24 2020, 6:15 AM
ymandel closed D84310: [libTooling] Add assorted `EditGenerator` combinators..
Jul 24 2020, 6:14 AM · Restricted Project
ymandel updated the diff for D84310: [libTooling] Add assorted `EditGenerator` combinators..

fixed lint

Jul 24 2020, 5:50 AM · Restricted Project

Jul 23 2020

ymandel added inline comments to D84310: [libTooling] Add assorted `EditGenerator` combinators..
Jul 23 2020, 10:02 AM · Restricted Project
Herald added a project to D84409: [libTooling] Add an `EditGenerator` that applies a rule throughout a bound node.: Restricted Project.
Jul 23 2020, 7:30 AM · Restricted Project

Jul 22 2020

Herald added a project to D84315: [libTooling] Add a `between` range-selector combinator.: Restricted Project.
Jul 22 2020, 5:34 AM · Restricted Project
ymandel updated the diff for D84310: [libTooling] Add assorted `EditGenerator` combinators..

revert unrelated change in RangeSelector.h

Jul 22 2020, 5:08 AM · Restricted Project
Herald added a project to D84310: [libTooling] Add assorted `EditGenerator` combinators.: Restricted Project.
Jul 22 2020, 5:06 AM · Restricted Project

Jul 21 2020

ymandel committed rGe5b3202b6f94: [libTooling] In Clang Transformer, change `Metadata` field to deferred… (authored by asoffer).
[libTooling] In Clang Transformer, change `Metadata` field to deferred…
Jul 21 2020, 11:07 AM
ymandel closed D83820: Change metadata to deferred evalutaion in Clang Transformer..
Jul 21 2020, 11:07 AM · Restricted Project
ymandel reopened D83820: Change metadata to deferred evalutaion in Clang Transformer..
Jul 21 2020, 5:48 AM · Restricted Project

Jul 20 2020

ymandel committed rGbd994b81d376: Revert "[libTooling] In Clang Transformer, change `Metadata` field to deferred… (authored by ymandel).
Revert "[libTooling] In Clang Transformer, change `Metadata` field to deferred…
Jul 20 2020, 9:18 PM
ymandel added a reverting change for rGc0b8954ecba5: [libTooling] In Clang Transformer, change `Metadata` field to deferred…: rGbd994b81d376: Revert "[libTooling] In Clang Transformer, change `Metadata` field to deferred….
Jul 20 2020, 9:18 PM
ymandel committed rGc0b8954ecba5: [libTooling] In Clang Transformer, change `Metadata` field to deferred… (authored by ymandel).
[libTooling] In Clang Transformer, change `Metadata` field to deferred…
Jul 20 2020, 9:18 PM
ymandel closed D83820: Change metadata to deferred evalutaion in Clang Transformer..
Jul 20 2020, 9:18 PM · Restricted Project

Jul 16 2020

ymandel added inline comments to D83820: Change metadata to deferred evalutaion in Clang Transformer..
Jul 16 2020, 12:27 PM · Restricted Project
ymandel added inline comments to D83820: Change metadata to deferred evalutaion in Clang Transformer..
Jul 16 2020, 12:26 PM · Restricted Project
ymandel accepted D83868: Use TestClangConfig in AST Matchers tests and run them in more configurations.

Thanks for doing this change! I think it's a great step forward for the testing robustness. I'm scared by how many fixme's we have in this file, but at least they're now explicit, rather than implicit. :)

Jul 16 2020, 6:14 AM · Restricted Project
ymandel added inline comments to D83820: Change metadata to deferred evalutaion in Clang Transformer..
Jul 16 2020, 5:33 AM · Restricted Project

Jul 15 2020

ymandel added a comment to D83820: Change metadata to deferred evalutaion in Clang Transformer..

Looks like your changes to the .cpp and test files were reverted...

Jul 15 2020, 12:35 PM · Restricted Project
ymandel accepted D83820: Change metadata to deferred evalutaion in Clang Transformer..
Jul 15 2020, 8:34 AM · Restricted Project

Jul 14 2020

ymandel accepted D83700: Fix test for the hasExternalFormalLinkage matcher.
Jul 14 2020, 6:24 AM · Restricted Project

Jul 9 2020

ymandel accepted D83301: [clang-tidy] More strict on matching the standard memset function in memset-usage check..
Jul 9 2020, 7:40 AM · Restricted Project

Jul 7 2020

ymandel added a comment to D82278: Fix traversal over CXXConstructExpr in Syntactic mode.

Thanks for this fix!

Jul 7 2020, 1:02 PM · Restricted Project

Jul 1 2020

ymandel committed rGecfa0b24189a: [libTooling] Fix `maybeExtendRange` to support `CharRange`s. (authored by gmatute).
[libTooling] Fix `maybeExtendRange` to support `CharRange`s.
Jul 1 2020, 2:05 PM
ymandel closed D82901: [libTooling] Fix `maybeExtendRange` to support `CharRange`s..
Jul 1 2020, 2:05 PM · Restricted Project
ymandel added a comment to D82906: [flang][openmp] Use common Directive and Clause enum from llvm/Frontend.

Seems like this is causing a slew of compilation warnings, like:

[745/3847] Building CXX object tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/SemaConcept.cpp.o
In file included from /usr/local/google/home/yitzhakm/remote-build/llvm-git/llvm-project/clang/lib/Sema/SemaConcept.cpp:24:
/usr/local/google/home/yitzhakm/remote-build/llvm-git/llvm-project/clang/include/clang/AST/RecursiveASTVisitor.h:2994:14: warning: enumeration values 'OMPC_inbranch', 'OMPC_link', and 'OMPC_notinbranch' not handled in switch [-Wswitch]
  switch (C->getClauseKind()) {
             ^
1 warning generated.
[766/3847] Building CXX object tools/clang/lib/Parse/CMakeFiles/obj.clangParse.dir/ParseOpenMP.cpp.o
/usr/local/google/home/yitzhakm/remote-build/llvm-git/llvm-project/clang/lib/Parse/ParseOpenMP.cpp:1689:11: warning: 19 enumeration values not handled in switch: 'OMPD_distribute_parallel_do', 'OMPD_distribute_parallel_do_simd', 'OMPD_do'
... [-Wswitch]
  switch (DKind) {
          ^
/usr/local/google/home/yitzhakm/remote-build/llvm-git/llvm-project/clang/lib/Parse/ParseOpenMP.cpp:2078:11: warning: 19 enumeration values not handled in switch: 'OMPD_distribute_parallel_do', 'OMPD_distribute_parallel_do_simd', 'OMPD_do'
... [-Wswitch]
  switch (DKind) {
          ^
Jul 1 2020, 2:04 PM · Restricted Project, Restricted Project, Restricted Project
ymandel accepted D82921: Removed a RecursiveASTVisitor feature to visit operator kinds with different methods.

+1 Especially given how trivial it was to fix clang/lib/ARCMigrate/TransProperties.cpp, this functionality seems unjustified.

Jul 1 2020, 6:28 AM · Restricted Project

Jun 30 2020

ymandel updated subscribers of D82901: [libTooling] Fix `maybeExtendRange` to support `CharRange`s..
Jun 30 2020, 11:56 AM · Restricted Project
ymandel created D82901: [libTooling] Fix `maybeExtendRange` to support `CharRange`s..
Jun 30 2020, 11:56 AM · Restricted Project
ymandel accepted D82875: Added tests for RecursiveASTVisitor for AST nodes that are special cased.
Jun 30 2020, 9:44 AM · Restricted Project
ymandel committed rG9945bd591163: Add Metadata to Transformer tooling (authored by asoffer).
Add Metadata to Transformer tooling
Jun 30 2020, 8:07 AM
ymandel closed D82226: Add Metadata to Transformer tooling.
Jun 30 2020, 8:07 AM · Restricted Project
ymandel accepted D82226: Add Metadata to Transformer tooling.
Jun 30 2020, 7:00 AM · Restricted Project

Jun 29 2020

ymandel accepted D82760: RecursiveASTVisitor: inline a macro that is only used once.
Jun 29 2020, 7:31 AM · Restricted Project

Jun 26 2020

ymandel committed rG30deabf89f93: [libTooling] Improve error message from failure in selection Stencil (authored by ymandel).
[libTooling] Improve error message from failure in selection Stencil
Jun 26 2020, 9:50 AM
ymandel closed D82654: [libTooling] Improve error message from failure in selection Stencil.
Jun 26 2020, 9:50 AM · Restricted Project
ymandel accepted D82486: RecursiveASTVisitor: don't call WalkUp unnecessarily in post-order traversal.
Jun 26 2020, 8:10 AM · Restricted Project
ymandel committed rG056a539e570a: [libTooling] Rename overloaded `range` range selector. (authored by ymandel).
[libTooling] Rename overloaded `range` range selector.
Jun 26 2020, 7:38 AM
ymandel closed D82592: [libTooling] Rename overloaded `range` range selector..
Jun 26 2020, 7:37 AM · Restricted Project
ymandel added a comment to D82654: [libTooling] Improve error message from failure in selection Stencil.

Any chance for a test?

Jun 26 2020, 7:37 AM · Restricted Project
ymandel added a comment to D82226: Add Metadata to Transformer tooling.

I think the tradeoff here is
Dynamic typing -- faster compile times, type safety checked at run-time (in tests), lower maintenance cost
Templates -- Faster runtime, type safety checked at compile-time, better user expereience

For me, the more important part of the tradeoff is whether we will have one type or multiple. If we use Any, then all AtomicChange regardless of what produced them, will have the same type. If we use templates, then AtomicChange<T> is a different type from AtomicChange<U>. So based on that I think using Any is a better choice, since infrastructure code would want to handle with AtomicChange objects and not have to be implemented as a template over arbitrary metadata type.

Jun 26 2020, 7:37 AM · Restricted Project
ymandel created D82654: [libTooling] Improve error message from failure in selection Stencil.
Jun 26 2020, 6:30 AM · Restricted Project

Jun 25 2020

ymandel created D82592: [libTooling] Rename overloaded `range` range selector..
Jun 25 2020, 1:05 PM · Restricted Project
ymandel added a comment to D82226: Add Metadata to Transformer tooling.

Looks good! Only real question is one of design -- should we consider the (deeper) change of templating the various types rather than using dynamic typing? For that matter, the decision doesn't even have to be the same for both AtomicChange and the Transformer types. Transformer could be templated while AtomicChange uses any.

Jun 25 2020, 6:58 AM · Restricted Project
ymandel accepted D82485: Add tests for sequences of callbacks that RecursiveASTVisitor produces.
Jun 25 2020, 6:53 AM · Restricted Project