[libcxx-commits] [PATCH] D111283: [clang] template / auto deduction deduces common sugar

[Matheus Izvekov via Phabricator via libcxx-commits]

mizvekov added a comment.

In D111283#3733415 <https://reviews.llvm.org/D111283#3733415>, @v.g.vassilev wrote:

> Thanks for working on this! I left some comments. I did not look very deep in the patch but it seems quite consistent to the rest of the code in Sema.

Thanks!

> Can you provide some performance numbers for this patch. I believe that the less confident reviewers will be more comfortable letting that patch in.

Yeah, sounds good.



================
Comment at: clang/lib/AST/ASTContext.cpp:12129
+  return cast_or_null<T>(
+      getCommonDecl(const_cast<Decl *>(cast_or_null<Decl>(X)),
+                    const_cast<Decl *>(cast_or_null<Decl>(Y))));
----------------
v.g.vassilev wrote:
> Maybe `getCommonDecl` should take `const Decl*` instead of `const_cast`-ing here.
If getCommonDecl took `const Decl`, I believe I would still need to `const_cast` to `const Decl *` anyway, because otherwise we would end up in infinite recursion into this function template.

And I would then in addition need to const_cast in the return value, which would make the whole thing more complicated.


================
Comment at: clang/lib/AST/ASTContext.cpp:12408
+  case Type::BlockPointer: {
+    const auto *PX = cast<BlockPointerType>(X), *PY = cast<BlockPointerType>(Y);
+    return Ctx.getBlockPointerType(getCommonPointeeType(Ctx, PX, PY));
----------------
v.g.vassilev wrote:
> Defining decl groups does not seem common in the llvm/clang codebase. I don't think we have a specific rule discouraging that. However, I think stepping through with a debugger will be harder. Maybe make this and other occurrences more consistent to the rest of the codebase?
Though in these cases the expression is just a cast, you would so rarely need to step into them as there is very little interesting going on in there.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111283/new/

https://reviews.llvm.org/D111283