hans added a comment.
>> I think it would be safer to do the change purely as an optimization in
>> codegen (maybe we could add a new helper method that could also be used by
>> the warning).
>
> For "optimization in codegen", do you mean optimization after the IR is
> generated or like I did in `CodeGenFunction::EmitCXXTypeidExpr`?
I mean like you did in `CodeGenFunction::EmitCXXTypeidExpr`.
================
Comment at: clang/include/clang/AST/ExprCXX.h:843
+ bool isMostDerived(ASTContext &Context) const;
+
----------------
A comment would be good here.
================
Comment at: clang/lib/AST/ExprCXX.cpp:151
+ if (isTypeOperand())
+ return false;
+
----------------
I suppose it depends on what you want the semantics of this function to be, but
returning false here seems odd.. wouldn't true make as much sense?
If this function is not intended to be called for type operands, maybe assert
about that instead?
================
Comment at: clang/lib/AST/ExprCXX.cpp:154
+ const Expr *E = getExprOperand()->IgnoreParenNoopCasts(Context);
+ if (auto *DRE = dyn_cast<DeclRefExpr>(E)) {
+ QualType Ty = DRE->getDecl()->getType();
----------------
I think checking for DeclRefExpr isn't guaranteed to handle all cases, like
```
typeid(*&obj)
```
for example. That's okay, but it should be clear from the function comment (and
maybe the name) that this is a best-effort check, not a strong guarantee about
whether the expression refers to a most derived object.
================
Comment at: clang/test/SemaCXX/constant-expression-cxx2a.cpp:312
// expected-note@+1 {{typeid applied to object 'extern_b2' whose dynamic
type is not constant}}
- static_assert(&typeid(extern_b2) == &typeid(B2)); // expected-error
{{constant expression}}
+ static_assert(&typeid(*&extern_b2) == &typeid(B2)); // expected-error
{{constant expression}}
----------------
This appears to be changing semantics. It could be that this test is
unnecessary strict (that's my understanding), but usually these checks are
based on examples in the standard, or the current understanding of the
standard. I think it would be best to check with Richard before changing this.
Actually, I'm surprised this affected since you're only updating CodeGen, not
Sema. Is the static_assert really invoking Codegen?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87425/new/
https://reviews.llvm.org/D87425
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
