This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Don't assume name of libc++ inline namespace in LibCxxUnorderedMap
ClosedPublic

Authored by kastiglione on Sep 3 2022, 11:36 AM.

Details

Summary

Follow up to D117383, fixing the assumption that libc++ always uses __1 as
its inline namespace name.

Diff Detail

Event Timeline

kastiglione created this revision.Sep 3 2022, 11:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 3 2022, 11:36 AM
kastiglione requested review of this revision.Sep 3 2022, 11:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 3 2022, 11:36 AM
rupprecht accepted this revision.Sep 6 2022, 2:36 PM
rupprecht added a subscriber: rupprecht.

LGTM -- I patched this in to verify it fixes the test failure we saw.

However, and not something that needs to happen in this patch (you could though), but the test actually isn't failing anymore even without this patch, because D129386 changed the naming convention from __cc to __cc_ I can get the test to correctly fail without this patch by changing the test assertion to:

must_not_contain__cc = r'(?s)^(?!.*\b__cc_ = )'

I'm not sure if there's a less fragile way to write that test. You could assert the entire string instead of looking for sub-fields, perhaps. Maybe that's hard to do for an unordered collection though.

This revision is now accepted and ready to land.Sep 6 2022, 2:36 PM
rupprecht added inline comments.Sep 6 2022, 2:45 PM
lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
76

This might be overly generic: every libc++ std inline namespace I've seen is __[a-zA-Z0-9]+::, e.g. we should handle std::__foo::unordered_map but not std::foo::unordered_map. I don't know if we've codified that convention anywhere.

For comparison, see https://github.com/llvm/llvm-project/blob/839b436c93604e042f74050cf2adadd75f30e898/lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp#L580

Thanks for the report about the __cc, I can try to come up with something less fragile.

lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
76

Happy to require the leading __.

Expect inline namespace to be __[[:alnum:]]+::

I updated the test in D133395

rupprecht accepted this revision.Sep 7 2022, 1:02 PM

Thanks again! Still looks good.

lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
70

nit: this consumes just the inline namespace, so I think consumeInlineNamespace might be a better name. I don't feel that strongly though so I'll leave it up to you.

JDevlieghere added inline comments.Sep 7 2022, 5:16 PM
lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
70

Rather than modifying the passed-in string, would it make sense to return a llvm::StringRef?

kastiglione added inline comments.Sep 16 2022, 8:29 AM
lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp
70

@JDevlieghere My answer is twofold: It follows the "consume" APIs on StringRef. And I prefer to avoid find expressions where you assign to a var being used on the right side:

someVar = someVar.method(...)

If you'd like me to change it, I will.

70

@rupprecht I agree, consumeInlineNamespace it is.

Rename to consumeInlineNamespace