[Lldb-commits] [PATCH] D67774: [Mangle] Check ExternalASTSource before adding prefix to asm label names

Thu Sep 19 13:48:44 PDT 2019

teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

I wonder what's the motivation for making this a setting in the ExternalASTSource? Opposed to e.g. storing a bit in AsmLabelAttr, which we could squeeze in by maybe stealing a bit from `labelLength`? Having this as a global setting in the ExternalASTSource seems like it could end up in a lot of confusion when we refactor our ExternalASTSources and this unexpectedly breaks. Especially since multiplexing ExternalASTSources is a thing people do (including LLDB itself), so if one source wants this feature on and the other doesn't, then we are in a dead end.

Also I wonder what happens with our real declarations we get from Clang modules (ObjC for now and C++ in the future). They now also share this setting even though they could use asm labels for legitimate reasons (even though I'm not sure I ever saw one being used in practice).

One minor bug I found: You need to turn this on in  `SemaSourceWithPriorities` and maybe also `ExternalASTSourceWrapper` (see ASTUtils.h) otherwise this seems to regress when using C++ modules and mangling stuff for an expression (I couldn't use `ClangExternalASTSourceCommon` there as we needed a clang::SemaSource).

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67774/new/

https://reviews.llvm.org/D67774