[PATCH] D135495: [clang-tidy] handle pointers in `ExceptionAnalyzer`

[Domján Dániel via Phabricator via cfe-commits]

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