r.stahl abandoned this revision.
r.stahl added a comment.
Herald added a reviewer: shafik.
ASTContext::getParents should not be used this way. This use-case is solved by
function-scoped ParentMaps or AnalysisDeclContext::getParentMap. Discussion:
http://lists.llvm.org/pipermail/cfe-dev/2019-Apri
a_sidorin added a comment.
Hello Richard,
Thank you for clarification. However, I think that moving ParentMap into
libTooling is out of scope for this patch. Are you OK if this change will be
committed with adding a TODO or FIXME for this move?
https://reviews.llvm.org/D46940
_
r.stahl added a comment.
@rsmith Yes, this should generally better be handled outside of the ASTContext.
That would be out of scope of this patch. Is it fine to proceed with this one
for now?
For my usage scenario it actually is convenient this way. In my checker I use
getParents, but if I wou
rsmith added a comment.
This is not specific to the `ASTImporter`; any change to the AST after a call
to `getParents` would have similar problems. Generally, responsibility for
dealing with this must lie with the consumer of the parent map, not with the
`ASTContext`, since the `ASTContext` gene
r.stahl added a comment.
@rsmith do you have a chance to take a look or assign someone else?
https://reviews.llvm.org/D46940
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
aaron.ballman added a reviewer: rsmith.
aaron.ballman added a comment.
Adding Richard as he has more background in this area. In general, I think it's
okay - if we invalidate too often, we do lose some performance, but the
correctness should still be there. I'll let Richard provide the final sig
a.sidorin accepted this revision.
a.sidorin added a comment.
LGTM. Aaron, could you please confirm that AST changes are fine?
Comment at: include/clang/AST/ASTContext.h:638
+ReleaseParentMapEntries();
+PointerParents = nullptr;
+ }
I'd prefer to call r
martong accepted this revision.
martong added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: rnkovacs.
Ok, thanks for the explanation. Now it looks good to me.
https://reviews.llvm.org/D46940
___
cfe-commits m
r.stahl added inline comments.
Comment at: lib/AST/ASTImporter.cpp:6776
+ // been invalidated to avoid repeatedly calling this.
+ ToContext.invalidateParents();
+
martong wrote:
> Can an `Expr` has a parent too? If yes, why not invalidate the parents in
> `Imp
martong added inline comments.
Comment at: lib/AST/ASTImporter.cpp:6776
+ // been invalidated to avoid repeatedly calling this.
+ ToContext.invalidateParents();
+
Can an `Expr` has a parent too? If yes, why not invalidate the parents in
`Import(Expr*)` ?
I am
r.stahl updated this revision to Diff 147268.
r.stahl marked 2 inline comments as done.
r.stahl added a comment.
addressed review comments
https://reviews.llvm.org/D46940
Files:
include/clang/AST/ASTContext.h
lib/AST/ASTImporter.cpp
unittests/AST/ASTImporterTest.cpp
Index: unittests/AST/
r.stahl added inline comments.
Comment at: lib/AST/ASTImporter.cpp:6755
+ auto ToE = cast_or_null(Import(cast(FromE)));
+ if (ToE)
+ToContext.invalidateParents();
a.sidorin wrote:
> We already do the invalidation in Import(Stmt), so it looks redundant here.
a.sidorin added a comment.
Hello Rafael,
The patch is awesome. The only thing I'm uncomfortable with is that we call
invalidation on every stmt import - not only during the top-level one. Fixing
this requires separating entry point from internal imports and is out of scope
of this patch, but a
r.stahl created this revision.
r.stahl added reviewers: a.sidorin, klimek.
Herald added subscribers: cfe-commits, martong.
If an AST node is imported after a call to getParents in the ToCtx, it was no
longer possible to retrieve its parents since the old results were cached.
Valid types for getP
14 matches
Mail list logo
