This is an archive of the discontinued LLVM Phabricator instance.

[clang] Correct calculation of MemberExpr's dependence
ClosedPublic

Authored by Fznamznon on Jul 7 2023, 1:49 AM.

Details

Summary

Due to incorrect calculation false positive diagnostics were emitted.

Fixes https://github.com/llvm/llvm-project/issues/48731

Diff Detail

Event Timeline

Fznamznon created this revision.Jul 7 2023, 1:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 7 2023, 1:49 AM
Fznamznon requested review of this revision.Jul 7 2023, 1:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 7 2023, 1:49 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Fznamznon added inline comments.Jul 7 2023, 1:55 AM
clang/lib/AST/ComputeDependence.cpp
625

While implementing this patch I was following Richard's guide from https://github.com/llvm/llvm-project/issues/48731#issuecomment-1529150797 . The problem is, I didn't manage to write a test that would fail due to clang not taking into account NameInfo dependence. Turns out in most cases where it can be dependent either Base itself is dependent or CXXPseudoDestructorExpr/CXXDependentScopeMemberExpr is created inside the template function.

cor3ntin added inline comments.Jul 7 2023, 2:33 AM
clang/lib/AST/Expr.cpp
1760

Maybe we should do that now, by passing TemplateArgs to computeDependence?

Otherwise there is a good chance that fixme never gets fixed !

Fznamznon added inline comments.Jul 7 2023, 2:46 AM
clang/lib/AST/Expr.cpp
1760

computeDependence is called by the constructor of MemberExpr which doesn't accept template augments. In order to do that, I would need to modify the constructor of MemberExpr.
Is it still reasonable to do as a part of this fix?

cor3ntin added inline comments.Jul 7 2023, 2:55 AM
clang/lib/AST/Expr.cpp
1760

Oh, i missed that the fixme is pre-existing, NVM then, i think you can leave it as-is. Sorry!

cor3ntin accepted this revision.Jul 7 2023, 4:16 AM

I think this makes sense and it implements richard's suggestion.
However, it's missing a release note, can you add that before landing?
Thanks

This revision is now accepted and ready to land.Jul 7 2023, 4:16 AM

I think this makes sense and it implements richard's suggestion.
However, it's missing a release note, can you add that before landing?
Thanks

Thank you for the review.
I think I added a release note here - https://reviews.llvm.org/D154689#change-RNUd6wICb9iD . Is something else needed?

I think this makes sense and it implements richard's suggestion.
However, it's missing a release note, can you add that before landing?
Thanks

Thank you for the review.
I think I added a release note here - https://reviews.llvm.org/D154689#change-RNUd6wICb9iD . Is something else needed?

Gosh, it's really not my day... nah, you're good! Sorry

shafik accepted this revision.Jul 7 2023, 11:24 AM

LGTM, thank you for the quick fix!

Rebase, add tests for dependent NameInfo

This revision was landed with ongoing or failed builds.Jul 10 2023, 3:01 AM
This revision was automatically updated to reflect the committed changes.