steakhal wrote:
Claude suggests two nits:
- The new block fires in every context, not just return statements, while the
comment above `visitReturnValueElidingTemp` says the sibling elision is
"deliberately NOT applied to
call/constructor arguments". The move-based rationale genuinely is
context-independent, unlike the return-slot one, but the two blocks read as
contradicting each other. A sentence
saying why, plus a test with a Ref-from-Ref temporary in argument position,
would pin the intended scope.
- `if (!InnerQT.isNull())` is dead — `Expr::getType()` never yields a null
`QualType` for a well-formed expression. And the InitListExpr peel works only
because IgnoreParenCasts() already
strips `FullExpr` (`clang/include/clang/AST/IgnoreExpr.h:69`), which also makes
the `dyn_cast<ExprWithCleanups>` just below it unreachable; moving the peel
after that unwrap would make the
ordering look deliberate.
Claude also thinks that this patch doesn't really work for Derived to Base
conversion constructors of `RefPtr`:
```c++
struct Holder {
RefPtr<Derived> m_ptr;
const RefPtr<Derived> accessor() const { return m_ptr; } // cf.
MessageEvent::securityOrigin()
};
RefPtr<Base> [[clang::annotate_type("webkit.nodelete")]]
copyFromConstRefPtr(Holder &h) {
return { h.accessor() }; // diagnosed before this patch, silent after
}
```
It suggests to check if the ctor is a Move ctor using:
```c++
auto *Ctor = CE->getConstructor();
bool IsMove = Ctor->getNumParams() == 1 &&
Ctor->getParamDecl(0)->getType()->isRValueReferenceType();
```
And this should give you back the warning for `const RefPtr<Derived> →
RefPtr<Base>`.
Allegedly the mock headers don't have the `RefPtr(Ref<T>&&)` Derived to Base
conversion constructor to write a proper test for it with adding that.
https://github.com/llvm/llvm-project/pull/219074
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
