[PATCH] D87425: [CodeGen][typeid] Emit typeinfo directly if type is known at compile-time

[Hans Wennborg via Phabricator via cfe-commits]

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 at +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