This is an archive of the discontinued LLVM Phabricator instance.

Fix DataExtractor symbol conflict
ClosedPublic

Authored by emrekultursay on Mar 1 2022, 5:59 AM.

Details

Summary

There are two DataExtractors in scope: one from the llvm namespace
and one from the lldb_private namespace. Some Microsoft Visual C++
compilers (I tested with MSVC 14.23 specifically) cannot handle this
situation, and generate ambiguous symbol errors. This change fixes
this compile error.

Diff Detail

Event Timeline

emrekultursay requested review of this revision.Mar 1 2022, 5:59 AM
emrekultursay created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 5:59 AM
shafik accepted this revision.Mar 1 2022, 8:16 AM
shafik added a subscriber: shafik.

LGTM

This revision is now accepted and ready to land.Mar 1 2022, 8:16 AM

Hi shafik, can you also submit it? I don't have submit permission. Thanks.

I think the better solution here is to get rid of the using namespace llvm; in the implementation file instead.

Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 12:16 PM
shafik added a comment.Mar 1 2022, 1:09 PM

I think the better solution here is to get rid of the using namespace llvm; in the implementation file instead.

That is a good point. I did a quick look at the file DataFileCache.h and none of the files it includes directly use using namespace llvm;.

It looks like across llvm-project we have 54 headers that use using namespace llvm; I thought it was a lot more.

I personally dislike it even in .cpp files using llvm:: or lldb_private:: does not feel like an undue burden. Although the count for .cpp files is 4000+ 😱

labath added a subscriber: labath.Mar 1 2022, 1:21 PM

I think the better solution here is to get rid of the using namespace llvm; in the implementation file instead.

That is a good point. I did a quick look at the file DataFileCache.h and none of the files it includes directly use using namespace llvm;.

It looks like across llvm-project we have 54 headers that use using namespace llvm; I thought it was a lot more.

It looks like at least some of those are inside functions, which is kind of OK. But what's worse, some of those are not inside functions, which is 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱 😱. using directives in headers completely mangle the symbol lookups in any file which includes them.

Removed "using namespace llvm" from the implementation file
and qualified all associated references with "llvm::".

I think the better solution here is to get rid of the using namespace llvm; in the implementation file instead.

Done. PTAL.

emrekultursay retitled this revision from Qualify DataExtractor with lldb_private to Fix DataExtractor symbol conflict.Mar 2 2022, 2:28 AM

Thanks for cleaning up the implementation file. LGMT module removing the added lldb_private:: which I believe we no longer need.

lldb/include/lldb/Core/DataFileCache.h
164–165

I assume the header was never a problem and you did this to match the signature in the implementation file (which no longer needs the namespace qualifier without the using namespace llvm), unless one of the headers included here (transitively) includes a header that has the "😱 😱 😱" top level using namespace llvm;?

Reverted the changes to the header file.

emrekultursay marked an inline comment as done.Mar 3 2022, 12:50 AM

Thanks. PTAL.

JDevlieghere accepted this revision.Mar 3 2022, 8:47 AM

Thank you! LGTM

This revision was automatically updated to reflect the committed changes.