aaronpuchert planned changes to this revision. aaronpuchert added a comment.
Thanks to both of you for the reviews. I'll see what I can do about the arrows. My gut feeling is that using `{Member,ObjCIVarRef}Expr::isArrow` is the right way, but it's not yet obvious to me how. ================ Comment at: include/clang/Analysis/Analyses/ThreadSafetyCommon.h:400 til::SExpr *translateMemberExpr(const MemberExpr *ME, CallingContext *Ctx); + til::SExpr *translateObjCIVarRefExpr(const ObjCIvarRefExpr *ME, + CallingContext *Ctx); ---------------- aaron.ballman wrote: > `ME` is kind of a poor name; can you switch to `IVRE` like you used in the > implementation? Of course, that's a copy-and-paste error. ================ Comment at: lib/Analysis/ThreadSafetyCommon.cpp:362 + til::Project *P = new (Arena) til::Project(E, D); + if (hasCppPointerType(BE)) + P->setArrow(true); ---------------- rjmccall wrote: > aaron.ballman wrote: > > I feel like this will always return false. However, I wonder if > > `IVRE->getBase()->isArrow()` is more appropriate here? > The base of an `ObjCIvarRefExpr` will never directly be a C pointer type. I > don't know whether this data structure looks through casts, but it's > certainly *possible* to cast arbitrarily weird C stuff to ObjC pointer type. > On the other hand, by and large nobody actually ever does that, so I wouldn't > make a special case for it here. > > `ObjCIvarRefExpr` just has an `isArrow()` method directly if this is just > supposed to be capturing the difference between `.` and `->`. But then, so > does `MemberExpr`, and in that case you're doing this odd analysis of the > base instead of trusting `isArrow()`, so I don't know what to tell you to do. > > Overall it is appropriate to treat an ObjC ivar reference as a perfectly > ordinary projection. The only thing that's special about it is that the > actual offset isn't necessarily statically known, but ivars still behave as > if they're all independently declared, so I wouldn't worry about it. I had wondered about this as well, but when I changed `hasCppPointerType(BE)` to `ME->isArrow()` in the `MemberExpr` case, I got some test failures. For example: ``` struct Foo { int a __attribute__((guarded_by(mu))); Mutex mu; }; void f() { Foo foo; foo.a = 5; // \ // expected-warning{{writing variable 'a' requires holding mutex 'foo.mu' exclusively}} } ``` Instead we warn `writing variable 'a' requires holding mutex 'foo->mu' exclusively`. That's not right. The expression (`ME`) we are processing is `mu` from the attribute on `a`, which the compiler sees as `this->mu`. So we get `ME->isArrow() == true` and `ME->getBase()` is a `CXXThisExpr`. Basically the `translate*` functions do a substitution. They try to derive `foo.mu` from `this->mu` on the `a` member and the expression `foo.a`. The annotation `this->mu` is the expression we get as parameter, and `foo.a` is encoded in the `CallingContext`. The information whether `foo.a` is an arrow (it isn't) seems to be in the `CallingContext`'s `SelfArrow` member. Repository: rC Clang https://reviews.llvm.org/D52200 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits