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

Gábor Horváth via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Jan 16 10:29:38 PST 2023


xazax.hun accepted this revision.
xazax.hun added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:60
+// Checks if T1 is convertible to T2.
+static bool isMultiLevelConvertiblePointer(QualType P1, QualType P2,
+                                           unsigned CurrentLevel = 0,
----------------
isuckatcs wrote:
> xazax.hun wrote:
> > xazax.hun wrote:
> > > Did you take a look at `ASTContext::hasCvrSimilarType`? I wonder if it is already doing what you want. 
> > Oh, maybe it is not, but it might also make sense to take a look at `ASTContext::typesAreCompatible`.
> > Did you take a look at ASTContext::hasCvrSimilarType? I wonder if it is already doing what you want.
> 
> This function compares the types by removing the CVR qualifiers.
> 
> > Oh, maybe it is not, but it might also make sense to take a look at ASTContext::typesAreCompatible.
> 
> This one only compares the canonical types if the language is C++.
> 
> ```lang=c++
> if (getLangOpts().CPlusPlus)
>     return hasSameType(LHS, RHS);
> ----------------------------
> bool hasSameType(QualType T1, QualType T2) const {
>     return getCanonicalType(T1) == getCanonicalType(T2);
>   }
> 
> ```
Could you rename the arguments like `To`, `From` or `Exception`, `Catch` or something to make the direction of the conversion clearer?


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:95
+
+  if (P1->isArrayType() && P2->isArrayType()) {
+    // At every level that array type is involved in, at least
----------------
isuckatcs wrote:
> xazax.hun wrote:
> > If none of the functions I recommended works for you, I still strongly suggest reusing utilities from ASTContext, like `UnwrapSimilarTypes` and `UnwrapSimilarArrayTypes`.
> I didn't check all of the functions inside `ASTContext`, but here we have very specific rules, we need to check. One utility might help with one check or two, but won't replace the need to go ove all of them. Also I think it's easier to understand which section checks what, if I keep them separated.
> 
> In an ealier comment I link to [[ https://en.cppreference.com/w/cpp/language/implicit_conversion#Qualification_conversions | the section on cppreference ]], which discusses what these rules are. Also there I found one controversial example, that's only detected by MSVC. I wonder if you know why that happens.
I see. It is unfortunate that we cannot simply reuse the corresponding functionality from Sema. The code mostly looks good to me, apart from some nits.


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:111
+    // ... [or there is an array type of known bound in P1 and
+    // an array type of unknown bound in P2 (since C++20)] then
+    // there must be a 'const' at every single level (other than
----------------
If this rule only applies to C++20 and above, consider adding a language version check for LangOpts.


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:130-131
+  if (!P1->isPointerType() || !P2->isPointerType())
+    return convertible && P1->getUnqualifiedDesugaredType() ==
+                              P2->getUnqualifiedDesugaredType();
+
----------------
Looking at the documentation of DesugaredType:
```
This takes off typedefs, typeof's etc. If the outer level of the type is already concrete, it returns it unmodified. This is similar to getting the canonical type, but it doesn't remove all typedefs. For example, it returns "T*" as "T*", (not as "int*"), because the pointer is concrete.
```
I wonder if we actually want to compare canonical types rather than desugared types.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135495/new/

https://reviews.llvm.org/D135495



More information about the cfe-commits mailing list