-
Notifications
You must be signed in to change notification settings - Fork 341
[lldb] Extract logic out of GetFunctionDisplayName into a helper function #10766
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
base: swift/release/6.2
Are you sure you want to change the base?
[lldb] Extract logic out of GetFunctionDisplayName into a helper function #10766
Conversation
I'm not sure about the changes to the However, this is not what's happening prior to the patch, as the parameters are printed as well. This does not seem to break any tests in |
if (open_paren && generic && generic < open_paren) | ||
return std::string(cstr, generic); | ||
if (open_paren) | ||
return std::string(cstr, open_paren); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm why is this new logic needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We didn't previously do this in the NoArgs
case right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit confused by the original implementation of the NoArgs
case: if the frame is foo(a: 1)
I would expect the NoArgs
case to return foo
and nothing else. But in the original implementation, it returns foo(a: Int)
.
This additional logic is due to me splitting the the logic into 2 helper functions. Say we have foo<T>(a: T)
. GetFunctionName
will return foo
and GetFunctionDisplayArgs
will return <Int>(a=1)
.
Hence: no, we didn't previously do this in the NoArgs
case, but I think this was a mistake. Since there are no tests for the way backtraces are printed in the Swift plugin, I have no idea how much of a breaking change this is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see. In that case, if eNoArgs
was always broken, lets just remove that case entirely? That'll mean less code to change in the other PR (and we won't be breaking any users)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted the changes that were in the eNoArgs
case so that the PR does not break existing users. Is that what you meant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant we should be able to remove the case entirely. If it never worked, it won't break any users 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible that a client is using this case incorrectly? They would not get any string from this function if that's the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And lets just have 1 helper function that factors out only the stuff in the eNameWithArgs
case. Cause that's what you intended to do in #10710 anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible that a client is using this case incorrectly? They would not get any string from this function if that's the case.
You're saying, if we removed the case, then a client would just not get any string if eFunctionNameNoArgs
was used? Yea if that's the case lets just leave it alone for now
Co-authored-by: Michael Buch <[email protected]>
@swift-ci test |
This patch splits the logic inside
SwiftLanguage::GetFunctionDisplayName
into reusable helper functions.SwiftLanguage::GetFunctionName
returns the name of the function as a string.SwiftLanguage::GetFunctionDisplayArgs
returns the generics and arguments of the function as a string.This will be needed for #10710