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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
