erichkeane added a comment. Not a great reviewer here as I'm not particularly sure what is going on, but I 'looked at the clang parts.
================ Comment at: clang/include/clang/AST/ASTContext.h:2828 + static bool isQualificationConvertiblePointer(QualType From, QualType To, + LangOptions LangOpts, ---------------- Please document what this is doing... ================ Comment at: clang/include/clang/AST/ASTContext.h:2829 + static bool isQualificationConvertiblePointer(QualType From, QualType To, + LangOptions LangOpts, + unsigned CurrentLevel = 0, ---------------- Why is this static if you need lang-opts? This should be retrieved by the ASTContext itself. ================ Comment at: clang/include/clang/AST/ASTContext.h:2831 + unsigned CurrentLevel = 0, + bool IsToConstSoFar = false); + ---------------- What does this name mean here? It isn't clear to me. ================ Comment at: clang/include/clang/AST/Type.h:7364 + else if (isArrayType()) + return getAsArrayTypeUnsafe()->getElementType(); + ---------------- This changes the meaning here, and this is a commonly used thing. Why are you doing this? ================ Comment at: clang/lib/AST/ASTContext.cpp:13465 +// +// The function should only be called in C++ mode. +bool ASTContext::isQualificationConvertiblePointer(QualType From, QualType To, ---------------- Perhaps this should be asserted on! ================ Comment at: clang/lib/AST/ASTContext.cpp:13466 +// The function should only be called in C++ mode. +bool ASTContext::isQualificationConvertiblePointer(QualType From, QualType To, + LangOptions LangOpts, ---------------- I find myself shocked we don't have something like this already, but what do we mean by 'qualification convertible'? Is that a term of art I'm missing? ================ Comment at: clang/lib/AST/ASTContext.cpp:13485 + + if (!To->isPointerType() || + !(From->canDecayToPointerType() || From->isPointerType())) ---------------- I would expect at least the 'to' here to assert as well. Passing a 'not pointer' as the 'two' when youre testing 'convertible pointer' is odd and a mistake? ================ Comment at: clang/lib/AST/ASTContext.cpp:13489 + + auto FromPointeeTy = From->getPointeeOrArrayElementQualType(); + auto ToPointeeTy = To->getPointeeOrArrayElementQualType(); ---------------- debatable whether we can use 'auto' here. I'd lean toward 'no'. ================ Comment at: clang/lib/AST/ASTContext.cpp:13492 + + return isQualificationConvertiblePointer(FromPointeeTy, ToPointeeTy, + LangOpts, CurrentLevel + 1, true); ---------------- I think the recursion here is making this too complicated iwth the "IsConstSoFar" and "Level", I'd probably suggest splitting these tasks up. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135495/new/ https://reviews.llvm.org/D135495 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits