Skip to content

[lldb] Use early returns in TypeSystemClang::IsPossibleDynamicType #137630

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 29, 2025

Conversation

labath
Copy link
Collaborator

@labath labath commented Apr 28, 2025

Remove a couple of levels of indentation before I need to add another one to fix a bug.

Remove a couple of levels of indentation before I need to add another
one to fix a bug.
@labath labath requested a review from Michael137 April 28, 2025 13:19
@labath labath requested a review from JDevlieghere as a code owner April 28, 2025 13:19
@llvmbot llvmbot added the lldb label Apr 28, 2025
@llvmbot
Copy link
Member

llvmbot commented Apr 28, 2025

@llvm/pr-subscribers-lldb

Author: Pavel Labath (labath)

Changes

Remove a couple of levels of indentation before I need to add another one to fix a bug.


Full diff: https://github.com/llvm/llvm-project/pull/137630.diff

1 Files Affected:

  • (modified) lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp (+88-109)
diff --git a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
index 59292f4b24af3..0dd9d8f75504e 100644
--- a/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ b/lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -3579,129 +3579,108 @@ bool TypeSystemClang::IsPossibleDynamicType(lldb::opaque_compiler_type_t type,
                                             CompilerType *dynamic_pointee_type,
                                             bool check_cplusplus,
                                             bool check_objc) {
+  if (dynamic_pointee_type)
+    dynamic_pointee_type->Clear();
+  if (!type)
+    return false;
+
+  auto set_dynamic_pointee_type = [&](clang::QualType type) {
+    if (dynamic_pointee_type)
+      dynamic_pointee_type->SetCompilerType(weak_from_this(),
+                                            type.getAsOpaquePtr());
+  };
+
   clang::QualType pointee_qual_type;
-  if (type) {
-    clang::QualType qual_type = RemoveWrappingTypes(GetCanonicalQualType(type));
-    bool success = false;
-    const clang::Type::TypeClass type_class = qual_type->getTypeClass();
-    switch (type_class) {
-    case clang::Type::Builtin:
-      if (check_objc &&
-          llvm::cast<clang::BuiltinType>(qual_type)->getKind() ==
-              clang::BuiltinType::ObjCId) {
-        if (dynamic_pointee_type)
-          dynamic_pointee_type->SetCompilerType(weak_from_this(), type);
-        return true;
-      }
-      break;
+  clang::QualType qual_type = RemoveWrappingTypes(GetCanonicalQualType(type));
+  switch (qual_type->getTypeClass()) {
+  case clang::Type::Builtin:
+    if (check_objc && llvm::cast<clang::BuiltinType>(qual_type)->getKind() ==
+                          clang::BuiltinType::ObjCId) {
+      set_dynamic_pointee_type(qual_type);
+      return true;
+    }
+    return false;
 
-    case clang::Type::ObjCObjectPointer:
-      if (check_objc) {
-        if (const auto *objc_pointee_type =
-                qual_type->getPointeeType().getTypePtrOrNull()) {
-          if (const auto *objc_object_type =
-                  llvm::dyn_cast_or_null<clang::ObjCObjectType>(
-                      objc_pointee_type)) {
-            if (objc_object_type->isObjCClass())
-              return false;
-          }
-        }
-        if (dynamic_pointee_type)
-          dynamic_pointee_type->SetCompilerType(
-              weak_from_this(),
-              llvm::cast<clang::ObjCObjectPointerType>(qual_type)
-                  ->getPointeeType()
-                  .getAsOpaquePtr());
-        return true;
+  case clang::Type::ObjCObjectPointer:
+    if (!check_objc)
+      return false;
+    if (const auto *objc_pointee_type =
+            qual_type->getPointeeType().getTypePtrOrNull()) {
+      if (const auto *objc_object_type =
+              llvm::dyn_cast_or_null<clang::ObjCObjectType>(
+                  objc_pointee_type)) {
+        if (objc_object_type->isObjCClass())
+          return false;
       }
-      break;
+    }
+    set_dynamic_pointee_type(
+        llvm::cast<clang::ObjCObjectPointerType>(qual_type)->getPointeeType());
+    return true;
 
-    case clang::Type::Pointer:
-      pointee_qual_type =
-          llvm::cast<clang::PointerType>(qual_type)->getPointeeType();
-      success = true;
-      break;
+  case clang::Type::Pointer:
+    pointee_qual_type =
+        llvm::cast<clang::PointerType>(qual_type)->getPointeeType();
+    break;
 
-    case clang::Type::LValueReference:
-    case clang::Type::RValueReference:
-      pointee_qual_type =
-          llvm::cast<clang::ReferenceType>(qual_type)->getPointeeType();
-      success = true;
-      break;
+  case clang::Type::LValueReference:
+  case clang::Type::RValueReference:
+    pointee_qual_type =
+        llvm::cast<clang::ReferenceType>(qual_type)->getPointeeType();
+    break;
 
+  default:
+    return false;
+  }
+
+  // Check to make sure what we are pointing to is a possible dynamic C++ type
+  // We currently accept any "void *" (in case we have a class that has been
+  // watered down to an opaque pointer) and virtual C++ classes.
+  switch (pointee_qual_type.getCanonicalType()->getTypeClass()) {
+  case clang::Type::Builtin:
+    switch (llvm::cast<clang::BuiltinType>(pointee_qual_type)->getKind()) {
+    case clang::BuiltinType::UnknownAny:
+    case clang::BuiltinType::Void:
+      set_dynamic_pointee_type(pointee_qual_type);
+      return true;
     default:
-      break;
+      return false;
     }
 
-    if (success) {
-      // Check to make sure what we are pointing too is a possible dynamic C++
-      // type We currently accept any "void *" (in case we have a class that
-      // has been watered down to an opaque pointer) and virtual C++ classes.
-      const clang::Type::TypeClass pointee_type_class =
-          pointee_qual_type.getCanonicalType()->getTypeClass();
-      switch (pointee_type_class) {
-      case clang::Type::Builtin:
-        switch (llvm::cast<clang::BuiltinType>(pointee_qual_type)->getKind()) {
-        case clang::BuiltinType::UnknownAny:
-        case clang::BuiltinType::Void:
-          if (dynamic_pointee_type)
-            dynamic_pointee_type->SetCompilerType(
-                weak_from_this(), pointee_qual_type.getAsOpaquePtr());
-          return true;
-        default:
-          break;
-        }
-        break;
-
-      case clang::Type::Record:
-        if (check_cplusplus) {
-          clang::CXXRecordDecl *cxx_record_decl =
-              pointee_qual_type->getAsCXXRecordDecl();
-          if (cxx_record_decl) {
-            bool is_complete = cxx_record_decl->isCompleteDefinition();
-
-            if (is_complete)
-              success = cxx_record_decl->isDynamicClass();
-            else {
-              if (std::optional<ClangASTMetadata> metadata =
-                      GetMetadata(cxx_record_decl))
-                success = metadata->GetIsDynamicCXXType();
-              else {
-                is_complete = GetType(pointee_qual_type).GetCompleteType();
-                if (is_complete)
-                  success = cxx_record_decl->isDynamicClass();
-                else
-                  success = false;
-              }
-            }
+  case clang::Type::Record: {
+    if (!check_cplusplus)
+      return false;
+    clang::CXXRecordDecl *cxx_record_decl =
+        pointee_qual_type->getAsCXXRecordDecl();
+    if (!cxx_record_decl)
+      return false;
 
-            if (success) {
-              if (dynamic_pointee_type)
-                dynamic_pointee_type->SetCompilerType(
-                    weak_from_this(), pointee_qual_type.getAsOpaquePtr());
-              return true;
-            }
-          }
-        }
-        break;
+    bool success;
+    if (cxx_record_decl->isCompleteDefinition())
+      success = cxx_record_decl->isDynamicClass();
+    else if (std::optional<ClangASTMetadata> metadata =
+                 GetMetadata(cxx_record_decl))
+      success = metadata->GetIsDynamicCXXType();
+    else if (GetType(pointee_qual_type).GetCompleteType())
+      success = cxx_record_decl->isDynamicClass();
+    else
+      success = false;
 
-      case clang::Type::ObjCObject:
-      case clang::Type::ObjCInterface:
-        if (check_objc) {
-          if (dynamic_pointee_type)
-            dynamic_pointee_type->SetCompilerType(
-                weak_from_this(), pointee_qual_type.getAsOpaquePtr());
-          return true;
-        }
-        break;
+    if (success)
+      set_dynamic_pointee_type(pointee_qual_type);
+    return success;
+  }
 
-      default:
-        break;
-      }
+  case clang::Type::ObjCObject:
+  case clang::Type::ObjCInterface:
+    if (check_objc) {
+      set_dynamic_pointee_type(pointee_qual_type);
+      return true;
     }
+    break;
+
+  default:
+    break;
   }
-  if (dynamic_pointee_type)
-    dynamic_pointee_type->Clear();
   return false;
 }
 

Comment on lines +3660 to +3662
else if (std::optional<ClangASTMetadata> metadata =
GetMetadata(cxx_record_decl))
success = metadata->GetIsDynamicCXXType();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the problematic part -- since we stopped searching for definitions eagerly, ClangASTMetadata::GetIsDynamicCXXType does not (always) hold the correct answer to this question.

@labath labath merged commit ebaeecc into llvm:main Apr 29, 2025
12 checks passed
@labath labath deleted the dynamic branch April 29, 2025 06:18
gizmondo pushed a commit to gizmondo/llvm-project that referenced this pull request Apr 29, 2025
…lvm#137630)

Remove a couple of levels of indentation before I need to add another
one to fix a bug.
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
…lvm#137630)

Remove a couple of levels of indentation before I need to add another
one to fix a bug.
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
…lvm#137630)

Remove a couple of levels of indentation before I need to add another
one to fix a bug.
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
…lvm#137630)

Remove a couple of levels of indentation before I need to add another
one to fix a bug.
GeorgeARM pushed a commit to GeorgeARM/llvm-project that referenced this pull request May 7, 2025
…lvm#137630)

Remove a couple of levels of indentation before I need to add another
one to fix a bug.
Ankur-0429 pushed a commit to Ankur-0429/llvm-project that referenced this pull request May 9, 2025
…lvm#137630)

Remove a couple of levels of indentation before I need to add another
one to fix a bug.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants