isuckatcs added a comment. > E.g., instead of asserting true/false, checking if the assignment would > compile.
This is actually biased. If the result of the compilation is different from the result we get from this function, it can also mean a bug in the compiler. Take a look at this example on godbolt <https://godbolt.org/z/vrErsqxqP>. The snippet here is only valid from C++20, but GCC compiles it even in C++17. ================ Comment at: clang/include/clang/AST/ASTContext.h:2828 + static bool isQualificationConvertiblePointer(QualType From, QualType To, + LangOptions LangOpts, ---------------- erichkeane wrote: > Please document what this is doing... This was actually documented, but at the definition. My bad. ================ Comment at: clang/include/clang/AST/ASTContext.h:2829 + static bool isQualificationConvertiblePointer(QualType From, QualType To, + LangOptions LangOpts, + unsigned CurrentLevel = 0, ---------------- erichkeane wrote: > Why is this static if you need lang-opts? This should be retrieved by the > ASTContext itself. By taking the language options externally, we might be able to produce more descriptive warning messages in the future. E.g.: `This conversion is only valid from C++20`, etc. Also calling this function doesn't depend on `ASTContext` any other way, so it can be called even if we don't have access to the `ASTContext` for some reason. I don't know however if it makes sense to worry about these uses at all. ================ Comment at: clang/include/clang/AST/ASTContext.h:2831 + unsigned CurrentLevel = 0, + bool IsToConstSoFar = false); + ---------------- erichkeane wrote: > What does this name mean here? It isn't clear to me. This is documented at the use site of this variable. I couldn't come up with a better name for it. ```lang=c++ // If at the current level To is more cv-qualified than From [...], // then there must be a 'const' at every single level (other than level zero) // of To up until the current level bool MoreCVQualified = To.getQualifiers().isStrictSupersetOf(From.getQualifiers()); if (MoreCVQualified) Convertible &= IsToConstSoFar; ``` ================ Comment at: clang/include/clang/AST/Type.h:7364 + else if (isArrayType()) + return getAsArrayTypeUnsafe()->getElementType(); + ---------------- erichkeane wrote: > This changes the meaning here, and this is a commonly used thing. Why are > you doing this? I missed that it changes the meaning. Though I need this use case, so I reverted these changes and created a static global function instead. ================ Comment at: clang/lib/AST/ASTContext.cpp:13465 +// +// The function should only be called in C++ mode. +bool ASTContext::isQualificationConvertiblePointer(QualType From, QualType To, ---------------- erichkeane wrote: > Perhaps this should be asserted on! I personally don't want to assert it, as it won't crash in C mode, it's just the fact that some rules here are different in C. E.g. (from [[ https://en.cppreference.com/w/cpp/language/implicit_conversion#Qualification_conversions | cppreference ]]): ```lang=c++ char** p = 0; const char* const * p2 = p; // error in C, OK in C++ ``` (The example above is checked properly but I didn't dig deeper into the other C rules, that's why I said that it shouldn't be called) ================ 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, ---------------- erichkeane wrote: > 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? > > I didn't come up with this name. It is what this conversion is called by the standard. It is §7.3.5 in [[ https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/n4849.pdf | N4849 ]] and you can also find it under the same name on [[ https://en.cppreference.com/w/cpp/language/implicit_conversion#Qualification_conversions | cppreference ]]. ================ Comment at: clang/lib/AST/ASTContext.cpp:13485 + + if (!To->isPointerType() || + !(From->canDecayToPointerType() || From->isPointerType())) ---------------- erichkeane wrote: > 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? Well, technically `MemberPointerType` is also accepted and it's not derived from `PointerType`. Though I added an assertion here, but I left the check so that we don't crash in release mode. ================ Comment at: clang/lib/AST/ASTContext.cpp:13492 + + return isQualificationConvertiblePointer(FromPointeeTy, ToPointeeTy, + LangOpts, CurrentLevel + 1, true); ---------------- erichkeane wrote: > I think the recursion here is making this too complicated iwth the > "IsConstSoFar" and "Level", I'd probably suggest splitting these tasks up. Splitting those up would just make it more complicated I think. We would need to perform nearly the same checks in both branches, though I keep thinking about it. 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
