Skip to content

[lldb] Devirtualize GetValueProperties (NFC) #126583

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
Feb 11, 2025

Conversation

JDevlieghere
Copy link
Member

Nobody is overriding GetValueProperties, so in practice we're always using m_collection_sp, which means we don't need to check the pointer. The temlated helpers were already operating on m_collection_sp directly so this makes the rest of the class consistent.

Nobody is overriding GetValueProperties, so in practice we're always
using `m_collection_sp`, which means we don't need to check the pointer.
The temlated helpers were already operating on `m_collection_sp`
directly so this makes the rest of the class consistent.
@llvmbot
Copy link
Member

llvmbot commented Feb 10, 2025

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

Nobody is overriding GetValueProperties, so in practice we're always using m_collection_sp, which means we don't need to check the pointer. The temlated helpers were already operating on m_collection_sp directly so this makes the rest of the class consistent.


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

2 Files Affected:

  • (modified) lldb/include/lldb/Core/UserSettingsController.h (+1-3)
  • (modified) lldb/source/Core/UserSettingsController.cpp (+7-26)
diff --git a/lldb/include/lldb/Core/UserSettingsController.h b/lldb/include/lldb/Core/UserSettingsController.h
index 32da7e05f7040f7..29e892fdba45bf3 100644
--- a/lldb/include/lldb/Core/UserSettingsController.h
+++ b/lldb/include/lldb/Core/UserSettingsController.h
@@ -38,9 +38,7 @@ class Properties {
 
   virtual ~Properties();
 
-  virtual lldb::OptionValuePropertiesSP GetValueProperties() const {
-    // This function is virtual in case subclasses want to lazily implement
-    // creating the properties.
+  lldb::OptionValuePropertiesSP GetValueProperties() const {
     return m_collection_sp;
   }
 
diff --git a/lldb/source/Core/UserSettingsController.cpp b/lldb/source/Core/UserSettingsController.cpp
index b57c1b0eef9b472..5408d64b406471f 100644
--- a/lldb/source/Core/UserSettingsController.cpp
+++ b/lldb/source/Core/UserSettingsController.cpp
@@ -40,64 +40,45 @@ Properties::~Properties() = default;
 lldb::OptionValueSP
 Properties::GetPropertyValue(const ExecutionContext *exe_ctx,
                              llvm::StringRef path, Status &error) const {
-  OptionValuePropertiesSP properties_sp(GetValueProperties());
-  if (properties_sp)
-    return properties_sp->GetSubValue(exe_ctx, path, error);
-  return lldb::OptionValueSP();
+  return m_collection_sp->GetSubValue(exe_ctx, path, error);
 }
 
 Status Properties::SetPropertyValue(const ExecutionContext *exe_ctx,
                                     VarSetOperationType op,
                                     llvm::StringRef path,
                                     llvm::StringRef value) {
-  OptionValuePropertiesSP properties_sp(GetValueProperties());
-  if (properties_sp)
-    return properties_sp->SetSubValue(exe_ctx, op, path, value);
-  return Status::FromErrorString("no properties");
+  return m_collection_sp->SetSubValue(exe_ctx, op, path, value);
 }
 
 void Properties::DumpAllPropertyValues(const ExecutionContext *exe_ctx,
                                        Stream &strm, uint32_t dump_mask,
                                        bool is_json) {
-  OptionValuePropertiesSP properties_sp(GetValueProperties());
-  if (!properties_sp)
-    return;
-
   if (is_json) {
-    llvm::json::Value json = properties_sp->ToJSON(exe_ctx);
+    llvm::json::Value json = m_collection_sp->ToJSON(exe_ctx);
     strm.Printf("%s", llvm::formatv("{0:2}", json).str().c_str());
   } else
-    properties_sp->DumpValue(exe_ctx, strm, dump_mask);
+    m_collection_sp->DumpValue(exe_ctx, strm, dump_mask);
 }
 
 void Properties::DumpAllDescriptions(CommandInterpreter &interpreter,
                                      Stream &strm) const {
   strm.PutCString("Top level variables:\n\n");
 
-  OptionValuePropertiesSP properties_sp(GetValueProperties());
-  if (properties_sp)
-    return properties_sp->DumpAllDescriptions(interpreter, strm);
+  return m_collection_sp->DumpAllDescriptions(interpreter, strm);
 }
 
 Status Properties::DumpPropertyValue(const ExecutionContext *exe_ctx,
                                      Stream &strm,
                                      llvm::StringRef property_path,
                                      uint32_t dump_mask, bool is_json) {
-  OptionValuePropertiesSP properties_sp(GetValueProperties());
-  if (properties_sp) {
-    return properties_sp->DumpPropertyValue(exe_ctx, strm, property_path,
+  return m_collection_sp->DumpPropertyValue(exe_ctx, strm, property_path,
                                             dump_mask, is_json);
-  }
-  return Status::FromErrorString("empty property list");
 }
 
 size_t
 Properties::Apropos(llvm::StringRef keyword,
                     std::vector<const Property *> &matching_properties) const {
-  OptionValuePropertiesSP properties_sp(GetValueProperties());
-  if (properties_sp) {
-    properties_sp->Apropos(keyword, matching_properties);
-  }
+  m_collection_sp->Apropos(keyword, matching_properties);
   return matching_properties.size();
 }
 

@JDevlieghere JDevlieghere merged commit 918848d into llvm:main Feb 11, 2025
9 checks passed
@JDevlieghere JDevlieghere deleted the UserSettingsController branch February 11, 2025 17:51
Icohedron pushed a commit to Icohedron/llvm-project that referenced this pull request Feb 11, 2025
Nobody is overriding GetValueProperties, so in practice we're always
using `m_collection_sp`, which means we don't need to check the pointer.
The temlated helpers were already operating on `m_collection_sp`
directly so this makes the rest of the class consistent.
flovent pushed a commit to flovent/llvm-project that referenced this pull request Feb 13, 2025
Nobody is overriding GetValueProperties, so in practice we're always
using `m_collection_sp`, which means we don't need to check the pointer.
The temlated helpers were already operating on `m_collection_sp`
directly so this makes the rest of the class consistent.
joaosaffran pushed a commit to joaosaffran/llvm-project that referenced this pull request Feb 14, 2025
Nobody is overriding GetValueProperties, so in practice we're always
using `m_collection_sp`, which means we don't need to check the pointer.
The temlated helpers were already operating on `m_collection_sp`
directly so this makes the rest of the class consistent.
sivan-shani pushed a commit to sivan-shani/llvm-project that referenced this pull request Feb 24, 2025
Nobody is overriding GetValueProperties, so in practice we're always
using `m_collection_sp`, which means we don't need to check the pointer.
The temlated helpers were already operating on `m_collection_sp`
directly so this makes the rest of the class consistent.
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