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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits