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

[Vassil Vassilev via Phabricator via libcxx-commits]

v.g.vassilev added a comment.

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.

Can you provide some performance numbers for this patch. I believe that the less confident reviewers will be more comfortable letting that patch in. I think this work is important and should be merged before GSoC ends. It will allow clang to diagnose better errors in template instantiations even for cases such as llvm.org/PR12853



================
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))));
----------------
Maybe `getCommonDecl` should take `const Decl*` instead of `const_cast`-ing here.


================
Comment at: clang/lib/AST/ASTContext.cpp:12172
+    return TemplateArgument(
+        Ctx.getCommonSugaredType(X.getNullPtrType(), Y.getNullPtrType()), true);
+  default:
----------------



================
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));
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111283