Skip to content

Conversation

@jimingham
Copy link
Collaborator

No description provided.

@llvmbot
Copy link
Member

llvmbot commented Sep 12, 2024

@llvm/pr-subscribers-lldb

Author: None (jimingham)

Changes

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

1 Files Affected:

  • (modified) lldb/docs/resources/sbapi.rst (+11)
diff --git a/lldb/docs/resources/sbapi.rst b/lldb/docs/resources/sbapi.rst
index cf32cc6c815581..4ca3909e0f2919 100644
--- a/lldb/docs/resources/sbapi.rst
+++ b/lldb/docs/resources/sbapi.rst
@@ -72,6 +72,17 @@ building the LLDB framework for macOS, the headers are processed with
 ``unifdef`` prior to being copied into the framework bundle to remove macros
 involving SWIG.
 
+Another good principle when adding SB API methods is: if you find yourself
+implementing a significant algorithm in the SB API method, you should not do
+that, but instead look for and then add it - if not found - as a method in the
+underlying lldb_private class, and then call that from your SB API method.
+If it was a useful algorithm, it's very likely it already exists
+because the lldb_private code also needed to do it.  And if it doesn't at
+present, if it was a useful thing to do, it's likely someone will later need
+it in lldb_private and then we end up with two implementations of the same
+algorithm.  If we keep the SB API code to just what's needed to manage the SB
+objects and requests, we won't get into this situation.
+
 Lifetime
 --------
 Many SB API methods will return strings in the form of ``const char *`` values.

Copy link
Contributor

@jeffreytan81 jeffreytan81 left a comment

Choose a reason for hiding this comment

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

Now I wish we can feed this kind of information to AI and let AI code review and catch it :-)

@jimingham jimingham merged commit 02d8813 into llvm:main Sep 13, 2024
@jimingham jimingham deleted the sb-api-comment branch September 13, 2024 17:18
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.

4 participants