[LLDB] Add more helper functions to CompilerType class (second try). #73472
Conversation
This adds 23 new helper functions to LLDB's CompilerType class, things
like IsSmartPtrType, IsPromotableIntegerType,
GetNumberofNonEmptyBaseClasses, and GetTemplateArgumentType (to name a
few).
It also has run clang-format on the files CompilerType.{h,cpp}.
These helper functions are needed as part of the implementation for
the Data Inspection Language, (see
https://discourse.llvm.org/t/rfc-data-inspection-language/69893).
|
@llvm/pr-subscribers-lldb Author: None (cmtice) ChangesThis adds 23 new helper functions to LLDB's CompilerType class, things like IsSmartPtrType, IsPromotableIntegerType, It also has run clang-format on the files CompilerType.{h,cpp}. These helper functions are needed as part of the implementation for the Data Inspection Language, (see Full diff: https://github.com/llvm/llvm-project/pull/73472.diff 2 Files Affected:
diff --git a/lldb/include/lldb/Symbol/CompilerType.h b/lldb/include/lldb/Symbol/CompilerType.h
index 0a9533a1ac0efc1..a3331ad3269c01d 100644
--- a/lldb/include/lldb/Symbol/CompilerType.h
+++ b/lldb/include/lldb/Symbol/CompilerType.h
@@ -112,9 +112,7 @@ class CompilerType {
/// Tests.
/// \{
- explicit operator bool() const {
- return m_type_system.lock() && m_type;
- }
+ explicit operator bool() const { return m_type_system.lock() && m_type; }
bool IsValid() const { return (bool)*this; }
@@ -194,6 +192,54 @@ class CompilerType {
bool IsTypedefType() const;
bool IsVoidType() const;
+
+ bool IsSmartPtrType() const;
+
+ bool IsInteger() const;
+
+ bool IsFloat() const;
+
+ bool IsEnumerationType() const;
+
+ bool IsUnscopedEnumerationType() const;
+
+ bool IsIntegerOrUnscopedEnumerationType() const;
+
+ bool IsSigned() const;
+
+ bool IsNullPtrType() const;
+
+ bool IsBoolean() const;
+
+ bool IsEnumerationIntegerTypeSigned() const;
+
+ bool IsScalarOrUnscopedEnumerationType() const;
+
+ bool IsPromotableIntegerType() const;
+
+ bool IsPointerToVoid() const;
+
+ bool IsRecordType() const;
+
+ bool IsVirtualBase(CompilerType target_base, CompilerType *virtual_base,
+ bool carry_virtual = false) const;
+
+ bool IsContextuallyConvertibleToBool() const;
+
+ bool IsBasicType() const;
+
+ std::string TypeDescription();
+
+ bool CompareTypes(CompilerType rhs) const;
+
+ const char *GetTypeTag();
+
+ uint32_t GetNumberOfNonEmptyBaseClasses();
+
+ CompilerType GetTemplateArgumentType(uint32_t idx);
+
+ CompilerType GetSmartPtrPointeeType();
+
/// \}
/// Type Completion.
@@ -436,8 +482,8 @@ class CompilerType {
ExecutionContextScope *exe_scope);
/// Dump to stdout.
- void DumpTypeDescription(lldb::DescriptionLevel level =
- lldb::eDescriptionLevelFull) const;
+ void DumpTypeDescription(
+ lldb::DescriptionLevel level = lldb::eDescriptionLevelFull) const;
/// Print a description of the type to a stream. The exact implementation
/// varies, but the expectation is that eDescriptionLevelFull returns a
diff --git a/lldb/source/Symbol/CompilerType.cpp b/lldb/source/Symbol/CompilerType.cpp
index 78cc8dad94a9c5f..854d6cab01b508e 100644
--- a/lldb/source/Symbol/CompilerType.cpp
+++ b/lldb/source/Symbol/CompilerType.cpp
@@ -54,7 +54,7 @@ bool CompilerType::IsArrayType(CompilerType *element_type_ptr, uint64_t *size,
if (IsValid())
if (auto type_system_sp = GetTypeSystem())
return type_system_sp->IsArrayType(m_type, element_type_ptr, size,
- is_incomplete);
+ is_incomplete);
if (element_type_ptr)
element_type_ptr->Clear();
@@ -157,7 +157,8 @@ bool CompilerType::IsBlockPointerType(
CompilerType *function_pointer_type_ptr) const {
if (IsValid())
if (auto type_system_sp = GetTypeSystem())
- return type_system_sp->IsBlockPointerType(m_type, function_pointer_type_ptr);
+ return type_system_sp->IsBlockPointerType(m_type,
+ function_pointer_type_ptr);
return false;
}
@@ -249,7 +250,7 @@ bool CompilerType::IsPossibleDynamicType(CompilerType *dynamic_pointee_type,
if (IsValid())
if (auto type_system_sp = GetTypeSystem())
return type_system_sp->IsPossibleDynamicType(m_type, dynamic_pointee_type,
- check_cplusplus, check_objc);
+ check_cplusplus, check_objc);
return false;
}
@@ -302,6 +303,256 @@ bool CompilerType::IsBeingDefined() const {
return false;
}
+bool CompilerType::IsSmartPtrType() const {
+ // These regular expressions cover shared, unique and weak pointers both from
+ // stdlibc++ and libc+++.
+
+ static llvm::Regex k_libcxx_std_unique_ptr_regex(
+ "^std::__[[:alnum:]]+::unique_ptr<.+>(( )?&)?$");
+ static llvm::Regex k_libcxx_std_shared_ptr_regex(
+ "^std::__[[:alnum:]]+::shared_ptr<.+>(( )?&)?$");
+ static llvm::Regex k_libcxx_std_weak_ptr_regex(
+ "^std::__[[:alnum:]]+::weak_ptr<.+>(( )?&)?$");
+ //
+ static llvm::Regex k_libcxx_std_unique_ptr_regex_2(
+ "^std::unique_ptr<.+>(( )?&)?$");
+ static llvm::Regex k_libcxx_std_shared_ptr_regex_2(
+ "^std::shared_ptr<.+>(( )?&)?$");
+ static llvm::Regex k_libcxx_std_weak_ptr_regex_2(
+ "^std::weak_ptr<.+>(( )?&)?$");
+ //
+ llvm::StringRef name = GetTypeName();
+ return k_libcxx_std_unique_ptr_regex.match(name) ||
+ k_libcxx_std_shared_ptr_regex.match(name) ||
+ k_libcxx_std_weak_ptr_regex.match(name) ||
+ k_libcxx_std_unique_ptr_regex_2.match(name) ||
+ k_libcxx_std_shared_ptr_regex_2.match(name) ||
+ k_libcxx_std_weak_ptr_regex_2.match(name);
+}
+
+bool CompilerType::IsInteger() const {
+ // This is used when you don't care about the signedness of the integer.
+ bool is_signed;
+ return IsIntegerType(is_signed);
+}
+
+bool CompilerType::IsFloat() const {
+ uint32_t count = 0;
+ bool is_complex = false;
+ return IsFloatingPointType(count, is_complex);
+}
+
+bool CompilerType::IsEnumerationType() const {
+ // This is used when you don't care about the signedness of the enum.
+ bool is_signed;
+ return IsEnumerationType(is_signed);
+}
+
+bool CompilerType::IsUnscopedEnumerationType() const {
+ return IsEnumerationType() && !IsScopedEnumerationType();
+}
+
+bool CompilerType::IsIntegerOrUnscopedEnumerationType() const {
+ return IsInteger() || IsUnscopedEnumerationType();
+}
+
+bool CompilerType::IsSigned() const {
+ if (IsEnumerationType()) {
+ return IsEnumerationIntegerTypeSigned();
+ }
+ return GetTypeInfo() & lldb::eTypeIsSigned;
+}
+
+bool CompilerType::IsNullPtrType() const {
+ return GetCanonicalType().GetBasicTypeEnumeration() ==
+ lldb::eBasicTypeNullPtr;
+}
+
+bool CompilerType::IsBoolean() const {
+ return GetCanonicalType().GetBasicTypeEnumeration() == lldb::eBasicTypeBool;
+}
+
+bool CompilerType::IsEnumerationIntegerTypeSigned() const {
+ if (IsValid()) {
+ return GetEnumerationIntegerType().GetTypeInfo() & lldb::eTypeIsSigned;
+ }
+ return false;
+}
+
+bool CompilerType::IsScalarOrUnscopedEnumerationType() const {
+ return IsScalarType() || IsUnscopedEnumerationType();
+}
+
+bool CompilerType::IsPromotableIntegerType() const {
+ // Unscoped enums are always considered as promotable, even if their
+ // underlying type does not need to be promoted (e.g. "int").
+ if (IsUnscopedEnumerationType()) {
+ return true;
+ }
+
+ switch (GetCanonicalType().GetBasicTypeEnumeration()) {
+ case lldb::eBasicTypeBool:
+ case lldb::eBasicTypeChar:
+ case lldb::eBasicTypeSignedChar:
+ case lldb::eBasicTypeUnsignedChar:
+ case lldb::eBasicTypeShort:
+ case lldb::eBasicTypeUnsignedShort:
+ case lldb::eBasicTypeWChar:
+ case lldb::eBasicTypeSignedWChar:
+ case lldb::eBasicTypeUnsignedWChar:
+ case lldb::eBasicTypeChar16:
+ case lldb::eBasicTypeChar32:
+ return true;
+
+ default:
+ return false;
+ }
+}
+
+bool CompilerType::IsPointerToVoid() const {
+ if (!IsValid())
+ return false;
+
+ return IsPointerType() &&
+ GetPointeeType().GetBasicTypeEnumeration() == lldb::eBasicTypeVoid;
+}
+
+bool CompilerType::IsRecordType() const {
+ if (!IsValid())
+ return false;
+
+ return GetCanonicalType().GetTypeClass() &
+ (lldb::eTypeClassClass | lldb::eTypeClassStruct |
+ lldb::eTypeClassUnion);
+}
+
+// Checks whether `target_base` is a virtual base of `type` (direct or
+// indirect). If it is, stores the first virtual base type on the path from
+// `type` to `target_type`.
+bool CompilerType::IsVirtualBase(CompilerType target_base,
+ CompilerType *virtual_base,
+ bool carry_virtual) const {
+ if (CompareTypes(target_base)) {
+ return carry_virtual;
+ }
+
+ if (!carry_virtual) {
+ uint32_t num_virtual_bases = GetNumVirtualBaseClasses();
+ for (uint32_t i = 0; i < num_virtual_bases; ++i) {
+ uint32_t bit_offset;
+ auto base = GetVirtualBaseClassAtIndex(i, &bit_offset);
+ if (base.IsVirtualBase(target_base, virtual_base,
+ /*carry_virtual*/ true)) {
+ if (virtual_base) {
+ *virtual_base = base;
+ }
+ return true;
+ }
+ }
+ }
+
+ uint32_t num_direct_bases = GetNumDirectBaseClasses();
+ for (uint32_t i = 0; i < num_direct_bases; ++i) {
+ uint32_t bit_offset;
+ auto base = GetDirectBaseClassAtIndex(i, &bit_offset);
+ if (base.IsVirtualBase(target_base, virtual_base, carry_virtual)) {
+ return true;
+ }
+ }
+
+ return false;
+}
+
+bool CompilerType::IsContextuallyConvertibleToBool() const {
+ return IsScalarType() || IsUnscopedEnumerationType() || IsPointerType() ||
+ IsNullPtrType() || IsArrayType();
+}
+
+bool CompilerType::IsBasicType() const {
+ return GetCanonicalType().GetBasicTypeEnumeration() !=
+ lldb::eBasicTypeInvalid;
+}
+
+std::string CompilerType::TypeDescription() {
+ auto name = GetTypeName();
+ auto canonical_name = GetCanonicalType().GetTypeName();
+ if (name.IsEmpty() || canonical_name.IsEmpty()) {
+ return "''"; // should not happen
+ }
+ if (name == canonical_name) {
+ return llvm::formatv("'{0}'", name);
+ }
+ return llvm::formatv("'{0}' (aka '{1}')", name, canonical_name);
+}
+
+bool CompilerType::CompareTypes(CompilerType rhs) const {
+ if (*this == rhs)
+ return true;
+
+ const ConstString name = GetFullyUnqualifiedType().GetTypeName();
+ const ConstString rhs_name = rhs.GetFullyUnqualifiedType().GetTypeName();
+ return name == rhs_name;
+}
+
+const char *CompilerType::GetTypeTag() {
+ switch (GetTypeClass()) {
+ // clang-format off
+ case lldb::eTypeClassClass: return "class";
+ case lldb::eTypeClassEnumeration: return "enum";
+ case lldb::eTypeClassStruct: return "struct";
+ case lldb::eTypeClassUnion: return "union";
+ // clang-format on
+ default:
+ return "unknown";
+ }
+}
+
+uint32_t CompilerType::GetNumberOfNonEmptyBaseClasses() {
+ // Go through the base classes and count non-empty ones.
+ uint32_t ret = 0;
+ uint32_t num_direct_bases = GetNumDirectBaseClasses();
+
+ for (uint32_t i = 0; i < num_direct_bases; ++i) {
+ uint32_t bit_offset;
+ CompilerType base_type = GetDirectBaseClassAtIndex(i, &bit_offset);
+ if (base_type.GetNumFields() > 0 ||
+ base_type.GetNumberOfNonEmptyBaseClasses() > 0) {
+ ret += 1;
+ }
+ }
+ return ret;
+}
+
+CompilerType CompilerType::GetTemplateArgumentType(uint32_t idx) {
+ CompilerType empty_type;
+ if (!IsValid())
+ return empty_type;
+
+ CompilerType type;
+ const bool expand_pack = true;
+ switch (GetTemplateArgumentKind(idx, true)) {
+ case lldb::eTemplateArgumentKindType:
+ type = GetTypeTemplateArgument(idx, expand_pack);
+ break;
+ case lldb::eTemplateArgumentKindIntegral:
+ type = GetIntegralTemplateArgument(idx, expand_pack)->type;
+ break;
+ default:
+ break;
+ }
+ if (type.IsValid())
+ return type;
+ return empty_type;
+}
+
+CompilerType CompilerType::GetSmartPtrPointeeType() {
+ assert(IsSmartPtrType() &&
+ "the type should be a smart pointer (std::unique_ptr, std::shared_ptr "
+ "or std::weak_ptr");
+
+ return GetTemplateArgumentType(0);
+}
+
// Type Completion
bool CompilerType::GetCompleteType() const {
@@ -336,9 +587,9 @@ ConstString CompilerType::GetDisplayTypeName() const {
uint32_t CompilerType::GetTypeInfo(
CompilerType *pointee_or_element_compiler_type) const {
if (IsValid())
- if (auto type_system_sp = GetTypeSystem())
- return type_system_sp->GetTypeInfo(m_type,
- pointee_or_element_compiler_type);
+ if (auto type_system_sp = GetTypeSystem())
+ return type_system_sp->GetTypeInfo(m_type,
+ pointee_or_element_compiler_type);
return 0;
}
@@ -362,8 +613,9 @@ void CompilerType::SetCompilerType(lldb::TypeSystemWP type_system,
m_type = type;
}
-void CompilerType::SetCompilerType(CompilerType::TypeSystemSPWrapper type_system,
- lldb::opaque_compiler_type_t type) {
+void CompilerType::SetCompilerType(
+ CompilerType::TypeSystemSPWrapper type_system,
+ lldb::opaque_compiler_type_t type) {
m_type_system = type_system.GetSharedPointer();
m_type = type;
}
@@ -589,7 +841,7 @@ uint32_t CompilerType::GetNumChildren(bool omit_empty_base_classes,
if (IsValid())
if (auto type_system_sp = GetTypeSystem())
return type_system_sp->GetNumChildren(m_type, omit_empty_base_classes,
- exe_ctx);
+ exe_ctx);
return 0;
}
@@ -601,8 +853,7 @@ lldb::BasicType CompilerType::GetBasicTypeEnumeration() const {
}
void CompilerType::ForEachEnumerator(
- std::function<bool(const CompilerType &integer_type,
- ConstString name,
+ std::function<bool(const CompilerType &integer_type, ConstString name,
const llvm::APSInt &value)> const &callback) const {
if (IsValid())
if (auto type_system_sp = GetTypeSystem())
@@ -623,7 +874,8 @@ CompilerType CompilerType::GetFieldAtIndex(size_t idx, std::string &name,
if (IsValid())
if (auto type_system_sp = GetTypeSystem())
return type_system_sp->GetFieldAtIndex(m_type, idx, name, bit_offset_ptr,
- bitfield_bit_size_ptr, is_bitfield_ptr);
+ bitfield_bit_size_ptr,
+ is_bitfield_ptr);
return CompilerType();
}
@@ -647,7 +899,7 @@ CompilerType::GetDirectBaseClassAtIndex(size_t idx,
if (IsValid())
if (auto type_system_sp = GetTypeSystem())
return type_system_sp->GetDirectBaseClassAtIndex(m_type, idx,
- bit_offset_ptr);
+ bit_offset_ptr);
return CompilerType();
}
@@ -657,7 +909,7 @@ CompilerType::GetVirtualBaseClassAtIndex(size_t idx,
if (IsValid())
if (auto type_system_sp = GetTypeSystem())
return type_system_sp->GetVirtualBaseClassAtIndex(m_type, idx,
- bit_offset_ptr);
+ bit_offset_ptr);
return CompilerType();
}
@@ -738,7 +990,7 @@ size_t CompilerType::GetIndexOfChildMemberWithName(
if (IsValid() && !name.empty()) {
if (auto type_system_sp = GetTypeSystem())
return type_system_sp->GetIndexOfChildMemberWithName(
- m_type, name, omit_empty_base_classes, child_indexes);
+ m_type, name, omit_empty_base_classes, child_indexes);
}
return 0;
}
@@ -772,7 +1024,8 @@ std::optional<CompilerType::IntegralTemplateArgument>
CompilerType::GetIntegralTemplateArgument(size_t idx, bool expand_pack) const {
if (IsValid())
if (auto type_system_sp = GetTypeSystem())
- return type_system_sp->GetIntegralTemplateArgument(m_type, idx, expand_pack);
+ return type_system_sp->GetIntegralTemplateArgument(m_type, idx,
+ expand_pack);
return std::nullopt;
}
|
lldb/source/Symbol/CompilerType.cpp
Outdated
| "^std::__[[:alnum:]]+::shared_ptr<.+>(( )?&)?$"); | ||
| static llvm::Regex k_libcxx_std_weak_ptr_regex( | ||
| "^std::__[[:alnum:]]+::weak_ptr<.+>(( )?&)?$"); | ||
| // |
lldb/source/Symbol/CompilerType.cpp
Outdated
| "^std::shared_ptr<.+>(( )?&)?$"); | ||
| static llvm::Regex k_libcxx_std_weak_ptr_regex_2( | ||
| "^std::weak_ptr<.+>(( )?&)?$"); | ||
| // |
lldb/source/Symbol/CompilerType.cpp
Outdated
| } | ||
|
|
||
| bool CompilerType::IsInteger() const { | ||
| // This is used when you don't care about the signedness of the integer. |
There was a problem hiding this comment.
This should this be a Doxygen comment (///) in the header. Also applies to comments in the functions.
lldb/source/Symbol/CompilerType.cpp
Outdated
| if (IsValid()) { | ||
| return GetEnumerationIntegerType().GetTypeInfo() & lldb::eTypeIsSigned; | ||
| } |
There was a problem hiding this comment.
No braces around single line if.
There was a problem hiding this comment.
Done (for entire change).
|
|
||
| default: | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This likely needs a llvm_unreachable("All cases handled above."); to appease gcc.
There was a problem hiding this comment.
lldb/source/Symbol/CompilerType.cpp
Outdated
| if (CompareTypes(target_base)) { | ||
| return carry_virtual; | ||
| } |
There was a problem hiding this comment.
There are many other instances of this in the code below
There was a problem hiding this comment.
Done (everywhere, I think).
lldb/source/Symbol/CompilerType.cpp
Outdated
| auto name = GetTypeName(); | ||
| auto canonical_name = GetCanonicalType().GetTypeName(); | ||
| if (name.IsEmpty() || canonical_name.IsEmpty()) { | ||
| return "''"; // should not happen |
There was a problem hiding this comment.
Should not happen as in we can guarantee that (in which case this should be an assert) or as in this would only happen for broken input?
There was a problem hiding this comment.
I believe this will only happen for broken input; I'll update the comment to make that clear.
lldb/source/Symbol/CompilerType.cpp
Outdated
| // clang-format off | ||
| case lldb::eTypeClassClass: return "class"; | ||
| case lldb::eTypeClassEnumeration: return "enum"; | ||
| case lldb::eTypeClassStruct: return "struct"; | ||
| case lldb::eTypeClassUnion: return "union"; | ||
| // clang-format on |
There was a problem hiding this comment.
Is this really worth disabling clang format for? It seems like this would just end up on the line below.
There was a problem hiding this comment.
Fixed the formatting & removed the comments.
lldb/source/Symbol/CompilerType.cpp
Outdated
| // These regular expressions cover shared, unique and weak pointers both from | ||
| // stdlibc++ and libc+++. | ||
|
|
||
| static llvm::Regex k_libcxx_std_unique_ptr_regex( |
There was a problem hiding this comment.
What does the prefix k mean here?
There was a problem hiding this comment.
It indicates the that these are constants. I can remove it if you like?
There was a problem hiding this comment.
Oh I see, I don't mind it being there. Just hadn't seen it before. If they're constants though, then please mark them const as well.
lldb/source/Symbol/CompilerType.cpp
Outdated
| @@ -302,6 +303,256 @@ bool CompilerType::IsBeingDefined() const { | |||
| return false; | |||
| } | |||
|
|
|||
| bool CompilerType::IsSmartPtrType() const { | |||
| // These regular expressions cover shared, unique and weak pointers both from | |||
| // stdlibc++ and libc+++. | |||
There was a problem hiding this comment.
If it covers both, I would suggest changing the names of the variables to not have the word "libcxx" in them. Comments can get out of date or otherwise be misleading, this one feels confusing to read through.
There was a problem hiding this comment.
Done. Renamed the libstdc++ variable names to contain 'libstdc++' instead of 'libc++'.
lldb/source/Symbol/CompilerType.cpp
Outdated
|
|
||
| bool CompilerType::IsInteger() const { | ||
| // This is used when you don't care about the signedness of the integer. | ||
| bool is_signed; |
There was a problem hiding this comment.
Initialize this to something even if it's unused. Reasoning: somebody may refactor this in the future in such a way that this becomes a problem.
lldb/source/Symbol/CompilerType.cpp
Outdated
| // Checks whether `target_base` is a virtual base of `type` (direct or | ||
| // indirect). If it is, stores the first virtual base type on the path from |
There was a problem hiding this comment.
This seems like it would be more useful as a doxygen comment in the header.
There was a problem hiding this comment.
This comment should also explain what virtual_base and carry_virtual are
There was a problem hiding this comment.
Done (mostly). I still need to check on 'carry_virtual' and add a comment about it.
lldb/source/Symbol/CompilerType.cpp
Outdated
| if (name == canonical_name) { | ||
| return llvm::formatv("'{0}'", name); | ||
| } | ||
| return llvm::formatv("'{0}' (aka '{1}')", name, canonical_name); |
There was a problem hiding this comment.
Suggestion: Instead of "'foo' (aka 'bar')" maybe something like
"'foo' (canonically referred to as 'bar')"?
My reasoning is 'aka' does not specify where this information is "known" or who knows it. Maybe canonical is also the wrong word but I think it would be useful to tell folks more than just "this type is also known as this other type". Maybe it's just me though.
lldb/source/Symbol/CompilerType.cpp
Outdated
| } | ||
|
|
||
| CompilerType CompilerType::GetSmartPtrPointeeType() { | ||
| assert(IsSmartPtrType() && |
There was a problem hiding this comment.
An assertion feels a little strong here, lldb crashing because somebody called the wrong function in the wrong place is a bit extreme to me.
In my opinion, the fact that GetSmartPtrPointeeType can fail if this isn't a smart pointer means that we should change the interface. Instead of returning a CompilerType, maybe std::optional or llvm::Expected?
There was a problem hiding this comment.
Fixed. Instead of asserting, if it's not a smart pointer I call GetPointeeType.
| @@ -436,8 +482,8 @@ class CompilerType { | |||
| ExecutionContextScope *exe_scope); | |||
|
|
|||
| /// Dump to stdout. | |||
| void DumpTypeDescription(lldb::DescriptionLevel level = | |||
| lldb::eDescriptionLevelFull) const; | |||
There was a problem hiding this comment.
I understand the desire to fix formatting issues, but let's avoid doing them in a big patch like this. If things needs to be reverted, as this patch was, such a formatting change should not need to be reverted.
This can also cause unnecessary work for downstream forks in case of merge conflicts (the first patch, the revert, the second patch, etc).
There was a problem hiding this comment.
Personally I agree with you. GIthub pull request creation did not give me any choice: It would not allow me to create a PR that did not pass clang-format. If you know of some way to bypass that requirement, PLEASE tell me!!
There was a problem hiding this comment.
It's ugly, but there is this:
// clang-format off
...
// clang-format onI am sure that you knew that and were asking for a nicer workaround. I, too, would be interested in learning such a trick, if it existed.
There was a problem hiding this comment.
It would not allow me to create a PR that did not pass clang-format.
Can you be more specific here, you could not open a PR at all without it being formatted? If that's the case it doesn't seem like what anyone would have intended.
I've definitely seen PRs that are opened then after that the bot will comment that it needs formatting.
Your other workaround here would be to do a PR to reformat these files, then rebase this onto that after landing it. Review has already started though, so maybe do that as the last thing after approvals.
There was a problem hiding this comment.
DavidSpickett: I created the PR, sort of, but... if you look at the bottom of this PR (for example) there's a small section that says "All Checks Passed". If you open that, you will see that one of the checks is a clang-format check. My original PR did not pass that check, so github would not "finish" creating the PR, i.e. it would not allow me to ask for reviews on it.
There was a problem hiding this comment.
I could be wrong here, but AFAICT the bot should only check for formatting changes inside the diff of the commit, not inside the entire file. If this is not what the bot is doing, it is something we can probably improve upon.
In the meantime though, my suggestion is to manually inspect the bot output: if it is complaining about lines not touched by this commit, just ignore that. This is favorable to editing lines that are not related to the patch
There was a problem hiding this comment.
I see this comment in another PR
NB: not applying all the clang-format recommendations as it affects lines I'm not editing, and I don't want to pollute git-blame
That makes me think the bot is not running clang-format-diff? https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting
There was a problem hiding this comment.
Question: For this PR, do you want me to un-do the clang-format changes that don't relate to my changes?
lldb/source/Symbol/CompilerType.cpp
Outdated
| @@ -54,7 +54,7 @@ bool CompilerType::IsArrayType(CompilerType *element_type_ptr, uint64_t *size, | |||
| if (IsValid()) | |||
| if (auto type_system_sp = GetTypeSystem()) | |||
| return type_system_sp->IsArrayType(m_type, element_type_ptr, size, | |||
| is_incomplete); | |||
| is_incomplete); | |||
There was a problem hiding this comment.
same issue with formatting as mentioned before
lldb/source/Symbol/CompilerType.cpp
Outdated
| @@ -157,7 +157,8 @@ bool CompilerType::IsBlockPointerType( | |||
| CompilerType *function_pointer_type_ptr) const { | |||
| if (IsValid()) | |||
| if (auto type_system_sp = GetTypeSystem()) | |||
| return type_system_sp->IsBlockPointerType(m_type, function_pointer_type_ptr); | |||
| return type_system_sp->IsBlockPointerType(m_type, | |||
There was a problem hiding this comment.
same issue with formatting as mentioned before
lldb/source/Symbol/CompilerType.cpp
Outdated
| @@ -249,7 +250,7 @@ bool CompilerType::IsPossibleDynamicType(CompilerType *dynamic_pointee_type, | |||
| if (IsValid()) | |||
| if (auto type_system_sp = GetTypeSystem()) | |||
| return type_system_sp->IsPossibleDynamicType(m_type, dynamic_pointee_type, | |||
| check_cplusplus, check_objc); | |||
| check_cplusplus, check_objc); | |||
There was a problem hiding this comment.
same issue with formatting as mentioned before
lldb/source/Symbol/CompilerType.cpp
Outdated
| "^std::unique_ptr<.+>(( )?&)?$"); | ||
| static llvm::Regex k_libcxx_std_shared_ptr_regex_2( | ||
| "^std::shared_ptr<.+>(( )?&)?$"); | ||
| static llvm::Regex k_libcxx_std_weak_ptr_regex_2( |
There was a problem hiding this comment.
Let's make these consts
lldb/source/Symbol/CompilerType.cpp
Outdated
|
|
||
| bool CompilerType::IsInteger() const { | ||
| // This is used when you don't care about the signedness of the integer. | ||
| bool is_signed; |
|
|
||
| default: | ||
| return false; | ||
| } |
There was a problem hiding this comment.
lldb/source/Symbol/CompilerType.cpp
Outdated
| if (CompareTypes(target_base)) { | ||
| return carry_virtual; | ||
| } |
There was a problem hiding this comment.
There are many other instances of this in the code below
lldb/source/Symbol/CompilerType.cpp
Outdated
| // Checks whether `target_base` is a virtual base of `type` (direct or | ||
| // indirect). If it is, stores the first virtual base type on the path from |
lldb/source/Symbol/CompilerType.cpp
Outdated
| // Checks whether `target_base` is a virtual base of `type` (direct or | ||
| // indirect). If it is, stores the first virtual base type on the path from |
There was a problem hiding this comment.
This comment should also explain what virtual_base and carry_virtual are
| @@ -194,6 +192,54 @@ class CompilerType { | |||
| bool IsTypedefType() const; | |||
|
|
|||
| bool IsVoidType() const; | |||
|
|
|||
| bool IsSmartPtrType() const; | |||
There was a problem hiding this comment.
Some methods like this one could benefit from a Doxygen comment that explains what the semantics are.
| bool IsVirtualBase(CompilerType target_base, CompilerType *virtual_base, | ||
| bool carry_virtual = false) const; | ||
|
|
||
| bool IsContextuallyConvertibleToBool() const; |
There was a problem hiding this comment.
This seems extremely specific to the C language family. I don't think that's a problem, but it would be nice to at least document that this may be only defined in TypeSystemClang?
|
|
||
| CompilerType GetTemplateArgumentType(uint32_t idx); | ||
|
|
||
| CompilerType GetSmartPtrPointeeType(); |
There was a problem hiding this comment.
There seems to be some overlap between this and the C++ shared_ptr synthetic child provider. I think it would be good if either the formatter uses this functionality or if the intended user of this function uses the synthetic children instead.
There was a problem hiding this comment.
Could you elaborate on this a little bit more please. I'm still learning the LLDB type system, and I don't quite understand what you're asking me to do.
There was a problem hiding this comment.
I looked at the functions in LibcxxSharedPtrSyntheticFrontend (iPlugins/Language/CPlusPlus) -- I assume this is what you were referring to. None of the functions there do exactly what this function does, so I can't use one of them instead. The only place I saw there that might be able to use this function was in GetChildAtIndex... I could replace 'valobj_sp->GetCompilerType().GetTypeTemplateArgument(0).GetPointerType()' with 'valobj_sp->GetCompilerType().GetSmartPtrPointeeType()', but it looks like the current call would return something like "* FooType", whereas my call would return "FooType", so they're not quite the same, either.
So I don't see how to do what you're asking.
There was a problem hiding this comment.
Here you are adding a function that takes a CompilerType holding an STL smart pointer and ask for the pointee type. We also have synthetic child formatters used by frame variable that allow evaluating *my_shared_ptr. From that I was assuming that there must be some overlap between what the dataformatter provides and what this function does.
What I don't know is whether synthetic children provided by a formatter are a good fit for what you are trying to do, but maybe the inverse is true, and the existing data formatter should be implemented in terms of this new GetSmartPtrPointeeType() API.
I just want to make sure we're not introducing two places in LLDB that have to know about STL smart pointers while covering slightly different functionality.
There was a problem hiding this comment.
Note that this function returns the type of the shared pointer, whereas the synthetic child providers return values of that type. So if anything you would implement the formatter on top of this type recognition (though that only gets you a little way there, you also have to find where the value is in the smart pointer.)
It's a little awkward that these API's speak of "smart pointer" when they really mean "the system library provided memory managing smart pointer". But I can't think of a good name to make this explicit.
However, that brings us round to the synthetic child support for "smart pointers". That is a feature that's not limited to the standard library's notion of a smart pointer. It really just tells lldb how to interpret the -> in:
(lldb) frame var foo->bar
The implementation of this feature doesn't require any knowledge of what the arrow operator for foo does (it is not even required to have one). For instance, you could use it to have lldb log uses of the -> operator for this type in the expression parser, or whatever. The only requirement is that you define a synthetic child provider that emulates the -> operator, returning a ValueObject from the appropriate API in the provider.
So at some point someone could redo the STL smart pointer comprehension in lldb to use the synthetic child arrow operator emulation, that's a much broader feature than what this API is promising.
There was a problem hiding this comment.
I was mostly trying to say that the ValueObject support for "faking the arrow operator effects" is orthogonal to having the type system contain the notion of "smart pointers".
I also think the whole naming of this is confusing. What is a "smart pointer"? The IsSmartPtr states some intent about this:
/// This determines if the type is a shared, unique or weak pointer, either /// from stdlibc++ or libc+++. bool IsSmartPtrType() const;
But that is also a little odd. This is a general type system query but doesn't in fact have anything to do with the type system. Rather with a particular implementation in the C++ STL - which is orthogonal to the C++ type system... So it seems like an awfully C++ specific API to have in the Typesystem.
There was a problem hiding this comment.
Having a comment saying "This is defined only for C++ types" in a function from CompilerType.h seems wrong.
There was a problem hiding this comment.
I can remove these helper functions from the CompilerType class and put them into my parser class for DIL instead, if you think that would be better.
There was a problem hiding this comment.
Clarification: I meant "I can remove the helper functions related to smart pointers."
There was a problem hiding this comment.
Ok, I've removed them now.
Address most of the reviewer comments: - Fix formatting issues - Add/Move Doxygen function comments in header file. - Undo clang-format changes to code that I didn't touch. - Other requested fixes.
Update comment for "IsVirtualBase" member function as requested.
|
I believe i have addressed all the review comments. Please let me know if there's anything else I need to do and/or (hopefully) approve this so I can merge it. Thanks! |
|
They really do seem odd in the general CompilerType...
Jim
… On Dec 5, 2023, at 4:35 PM, cmtice ***@***.***> wrote:
@cmtice commented on this pull request.
In lldb/include/lldb/Symbol/CompilerType.h <#73472 (comment)>:
> +
+ bool IsContextuallyConvertibleToBool() const;
+
+ bool IsBasicType() const;
+
+ std::string TypeDescription();
+
+ bool CompareTypes(CompilerType rhs) const;
+
+ const char *GetTypeTag();
+
+ uint32_t GetNumberOfNonEmptyBaseClasses();
+
+ CompilerType GetTemplateArgumentType(uint32_t idx);
+
+ CompilerType GetSmartPtrPointeeType();
Clarification: I meant "I can remove the helper functions related to smart pointers."
—
Reply to this email directly, view it on GitHub <#73472 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW4JAG5WJJFUNJSL2HLYH64WZAVCNFSM6AAAAAA73I4PWOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONRWGM3TAOJXGY>.
You are receiving this because you are on a team that was mentioned.
|
Remove smart pointer functions from the new helper functions for CompilerType class.
|
I've removed the Smart pointer functions from this PR. Are there any other changes that I need to make? If not, could someone approve this? |
|
Ping! Could somebody please either approve this PR or tell me what other changes they would like to see? Pretty please? |
|
@bulbazord Actually, I think you're right. I use that in my smart pointer type code. I will pull that out as well (I overlooked it before). |
Remove GetTemplateArgumentType from list of new helper functions (it's too C++-specific).
|
Ok, I have now removed CompilerType::GetTemplateArgumentType from the new helper functions. |
| if (IsEnumerationType()) | ||
| return IsEnumerationIntegerTypeSigned(); | ||
|
|
||
| return GetTypeInfo() & lldb::eTypeIsSigned; |
There was a problem hiding this comment.
This function looks like it's working around a bug in GetTypeInfo?
There was a problem hiding this comment.
(i.e., could this be fixed by modifying TypeSystemClang::GetTypeInfo to return the sign bit on enums?
There was a problem hiding this comment.
@adrian-prantl Actually I think it already works properly without special-casing enums (I went back and looked and could not figure out why I had done this). I've cleaned it up now and removed the special casing. Are you OK with me committing this now?
Clean up implementation of IsSigned function.
This adds 23 new helper functions to LLDB's CompilerType class, things like IsSmartPtrType, IsPromotableIntegerType,
GetNumberofNonEmptyBaseClasses, and GetTemplateArgumentType (to name a few).
It also has run clang-format on the files CompilerType.{h,cpp}.
These helper functions are needed as part of the implementation for the Data Inspection Language, (see
https://discourse.llvm.org/t/rfc-data-inspection-language/69893).