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

[Erich Keane via Phabricator via cfe-commits]

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