Skip to content

[lldb][CPlusPlus] Always use CPlusPlusNameParser for parsing C++ function names #137072

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[lldb][CPlusPlus] Always use CPlusPlusNameParser for parsing C++ function names #137072

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
91fc35b
[lldb][CPlusPlus] Remove TrySimplifiedParse when hand-parsing C++ fun…
Michael137 Apr 23, 2025
45aeebe
fixup! clang-format
Michael137 Apr 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 9 additions & 83 deletions lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,34 +173,6 @@ static bool ReverseFindMatchingChars(const llvm::StringRef &s,
return false;
}

static bool IsTrivialBasename(const llvm::StringRef &basename) {
// Check that the basename matches with the following regular expression
// "^~?([A-Za-z_][A-Za-z_0-9]*)$" We are using a hand written implementation
// because it is significantly more efficient then using the general purpose
// regular expression library.
size_t idx = 0;
if (basename.starts_with('~'))
idx = 1;

if (basename.size() <= idx)
return false; // Empty string or "~"

if (!std::isalpha(basename[idx]) && basename[idx] != '_')
return false; // First character (after removing the possible '~'') isn't in
// [A-Za-z_]

// Read all characters matching [A-Za-z_0-9]
++idx;
while (idx < basename.size()) {
if (!std::isalnum(basename[idx]) && basename[idx] != '_')
break;
++idx;
}

// We processed all characters. It is a vaild basename.
return idx == basename.size();
}

/// Writes out the function name in 'full_name' to 'out_stream'
/// but replaces each argument type with the variable name
/// and the corresponding pretty-printed value
Expand Down Expand Up @@ -235,66 +207,20 @@ static bool PrettyPrintFunctionNameWithArgs(Stream &out_stream,
return true;
}

bool CPlusPlusLanguage::CxxMethodName::TrySimplifiedParse() {
// This method tries to parse simple method definitions which are presumably
// most comman in user programs. Definitions that can be parsed by this
// function don't have return types and templates in the name.
// A::B::C::fun(std::vector<T> &) const
size_t arg_start, arg_end;
llvm::StringRef full(m_full.GetCString());
llvm::StringRef parens("()", 2);
if (ReverseFindMatchingChars(full, parens, arg_start, arg_end)) {
m_arguments = full.substr(arg_start, arg_end - arg_start + 1);
if (arg_end + 1 < full.size())
m_qualifiers = full.substr(arg_end + 1).ltrim();

if (arg_start == 0)
return false;
size_t basename_end = arg_start;
size_t context_start = 0;
size_t context_end = full.rfind(':', basename_end);
if (context_end == llvm::StringRef::npos)
m_basename = full.substr(0, basename_end);
else {
if (context_start < context_end)
m_context = full.substr(context_start, context_end - 1 - context_start);
const size_t basename_begin = context_end + 1;
m_basename = full.substr(basename_begin, basename_end - basename_begin);
}

if (IsTrivialBasename(m_basename)) {
return true;
} else {
// The C++ basename doesn't match our regular expressions so this can't
// be a valid C++ method, clear everything out and indicate an error
m_context = llvm::StringRef();
m_basename = llvm::StringRef();
m_arguments = llvm::StringRef();
m_qualifiers = llvm::StringRef();
m_return_type = llvm::StringRef();
return false;
}
}
return false;
}

void CPlusPlusLanguage::CxxMethodName::Parse() {
if (!m_parsed && m_full) {
if (TrySimplifiedParse()) {
CPlusPlusNameParser parser(m_full.GetStringRef());
if (auto function = parser.ParseAsFunctionDefinition()) {
m_basename = function->name.basename;
m_context = function->name.context;
m_arguments = function->arguments;
m_qualifiers = function->qualifiers;
m_return_type = function->return_type;
m_parse_error = false;
} else {
CPlusPlusNameParser parser(m_full.GetStringRef());
if (auto function = parser.ParseAsFunctionDefinition()) {
m_basename = function->name.basename;
m_context = function->name.context;
m_arguments = function->arguments;
m_qualifiers = function->qualifiers;
m_return_type = function->return_type;
m_parse_error = false;
} else {
m_parse_error = true;
}
m_parse_error = true;
}

if (m_context.empty()) {
m_scope_qualified = std::string(m_basename);
} else {
Expand Down
4 changes: 4 additions & 0 deletions lldb/test/API/lang/cpp/operators/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ int main(int argc, char **argv) {
C *c2 = new C();
C *c3 = new C[3];

// clang-format off
//% self.expect("expr c->dummy", endstr=" 2324\n")
//% self.expect("expr c->*2", endstr=" 2\n")
//% self.expect("expr c + 44", endstr=" 44\n")
Expand Down Expand Up @@ -174,6 +175,9 @@ int main(int argc, char **argv) {
//% self.expect("expr (new struct C[1]); side_effect", endstr=" = 4\n")
//% self.expect("expr delete c2; side_effect", endstr=" = 1\n")
//% self.expect("expr delete[] c3; side_effect", endstr=" = 2\n")
//% self.expect("image lookup -n operator()", substrs=["C::operator()(int)"])
Copy link
Collaborator

@labath labath Apr 24, 2025

Choose a reason for hiding this comment

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

I take it this means that the "simplified" parser got operator() wrong somehow? Could you also add this to the unit tests in unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp

Copy link
Member Author

@Michael137 Michael137 Apr 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yea it was deducing that operator is the basename because it just does a simple search for () and then checks whether the rest of the name looks like a valid function identifier.

Could you also add this to the unit tests in unittests/Language/CPlusPlus/CPlusPlusLanguageTest.cpp

Yup!

//% self.expect("image lookup -n C::operator()", substrs=["C::operator()(int)"])
// clang-format on
delete c2;
delete[] c3;
return 0;
Expand Down
Loading