Re: [PATCH] D19144: Handle TemplateArgument in DynTypedNode comparison operations.
alexfh abandoned this revision. alexfh added a comment. Ok, let's drop this on the floor. http://reviews.llvm.org/D19144 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19144: Handle TemplateArgument in DynTypedNode comparison operations.
sbenza added a comment. > > We can proceed with this change if you want, but it is not required > > anymore. I don't know whether we need the extra complexity of > > `TemplateArgumentLess`. > > > If this patch is not going to help with performance, I'm happy to abandon it. It might help in the cases where you are doing hasAncestor with TemplateArgument bound nodes, but that is a small corner case and I don't think we need to optimize for it. http://reviews.llvm.org/D19144 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19144: Handle TemplateArgument in DynTypedNode comparison operations.
alexfh added a comment. In http://reviews.llvm.org/D19144#404234, @sbenza wrote: > Sent http://reviews.llvm.org/D19231 to fix the underlying bug in > `hasAncestor`. > We can proceed with this change if you want, but it is not required anymore. > I don't know whether we need the extra complexity of `TemplateArgumentLess`. If this patch is not going to help with performance, I'm happy to abandon it. http://reviews.llvm.org/D19144 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19144: Handle TemplateArgument in DynTypedNode comparison operations.
sbenza added a comment. Sent http://reviews.llvm.org/D19231 to fix the underlying bug in `hasAncestor`. We can proceed with this change if you want, but it is not required anymore. I don't know whether we need the extra complexity of `TemplateArgumentLess`. http://reviews.llvm.org/D19144 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19144: Handle TemplateArgument in DynTypedNode comparison operations.
alexfh added a comment. In http://reviews.llvm.org/D19144#402853, @sbenza wrote: > I think the bug is coming from `memoizedMatchesAncestorOfRecursively`. > `memoizedMatchesRecursively` has a special case at the top to skip the cache > if the node is not sortable. The other function should do that too. > Although the check is stale also because it is only checking for > memoizationData and not whether the node itself works for < and ==. > > Note that adding TemplateArgument to the function is ok, but that won't fix > the bug because we still have other nodes that are not comparable. With this I would be entering "I have no idea what I'm doing" land ;) Might make sense for you to take over the patch, if you know how to fix the issue. WDYT? http://reviews.llvm.org/D19144 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19144: Handle TemplateArgument in DynTypedNode comparison operations.
sbenza added a comment. To be more specific, the problem comes from the use of `hasAnscestor` (done by `isInTemplateInstantiation` ) while there is a `TemplateArgument` in the bound nodes. This tries to put the node into the cache. To trigger this easily you only need to have a hit in the cache. I think you can trigger it by adding a second line `boost::lexical_cast(42);` in the `string_as_T` test. That second hit should get a cache hit and trigger the bug. http://reviews.llvm.org/D19144 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D19144: Handle TemplateArgument in DynTypedNode comparison operations.
sbenza added a comment. I think the bug is coming from `memoizedMatchesAncestorOfRecursively`. `memoizedMatchesRecursively` has a special case at the top to skip the cache if the node is not sortable. The other function should do that too. Although the check is stale also because it is only checking for memoizationData and not whether the node itself works for < and ==. Note that adding TemplateArgument to the function is ok, but that won't fix the bug because we still have other nodes that are not comparable. Comment at: include/clang/AST/ASTTypeTraits.h:269 @@ -268,3 +268,3 @@ /// /// Supports comparison of nodes that support memoization. /// FIXME: Implement comparsion for other node types (currently this comment is stale. http://reviews.llvm.org/D19144 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
