This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Take StringRef names in GetChildAtNamePath (NFC)
ClosedPublic

Authored by kastiglione on May 31 2023, 8:37 AM.

Details

Summary

Following D151810, this changes GetChildAtNamePath to take a path of StringRef
values instead of ConstString.

Diff Detail

Event Timeline

kastiglione created this revision.May 31 2023, 8:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2023, 8:37 AM
kastiglione requested review of this revision.May 31 2023, 8:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2023, 8:37 AM
Michael137 accepted this revision.May 31 2023, 8:45 AM
Michael137 added a subscriber: Michael137.

lgtm

lldb/include/lldb/Core/ValueObject.h
483

Guessing removing the parameter is fine because no callers were actually passing it?

This revision is now accepted and ready to land.May 31 2023, 8:45 AM
JDevlieghere accepted this revision.May 31 2023, 8:47 AM
JDevlieghere added inline comments.
lldb/source/Core/ValueObject.cpp
434

I don’t think this qualifies for ‘auto’ according to the LLVM coding guidelines.

kastiglione added inline comments.May 31 2023, 9:22 AM
lldb/include/lldb/Core/ValueObject.h
483

I forgot I had made this change too. Correct, no callers were using this, so I removed it. I can imagine there could be a hypothetical case that uses it to determine a fallback action, but none do and it seems unlikely.

lldb/source/Core/ValueObject.cpp
434

I find the guidelines to be not specific enough. Case in point, this section shows auto being used in range for loops: https://llvm.org/docs/CodingStandards.html#beware-unnecessary-copies-with-auto

bulbazord accepted this revision.May 31 2023, 10:10 AM
bulbazord added inline comments.
lldb/source/Core/ValueObject.cpp
434

nit: mark name as const -> const auto name.
I suppose it's ok if it's a copy since it's a StringRef and it's less expensive to make the copy?

kastiglione added inline comments.May 31 2023, 10:37 AM
lldb/source/Core/ValueObject.cpp
434

yes I figure a StringRef is cheap to copy.

Change auto to StringRef