aprantl accepted this revision. aprantl added inline comments. This revision is now accepted and ready to land.
================ Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:9818 + if (ClangASTMetadata *metadata = GetMetadata(fun_decl)) + return metadata->GetObjectPtrLanguage(); + } ---------------- kastiglione wrote: > kastiglione wrote: > > aprantl wrote: > > > Couple of questions for my understanding: > > > > > > 1. Do you see any opportunities for this to spectacularly do the wrong > > > thing in an ObjectiveC++ program by guessing the wrong language? I > > > suppose this just tries to do a best effort? > > > > > > 2. We're not using the DW_AT_APPLE_runtime_language attribute because > > > it's only attached to Objective-C classes? > > > > > > 3. We're not using the DW_AT_language of the DW_TAG_compile_unit because > > > it won't help us in an ObjC++ program? > > > We're not using the `DW_AT_APPLE_runtime_language` attribute because it's > > > only attached to Objective-C classes? > > > We're not using the `DW_AT_language` of the `DW_TAG_compile_unit` because > > > it won't help us in an ObjC++ program? > > > > I am not sure how to answer these too. While this function is new, the > > logic and this code path which calls `GetObjectPtrLanguage`, is taken from > > the existing implementation. In other words, how the debug info is made use > > of, hasn't changed. > > Do you see any opportunities for this to spectacularly do the wrong thing > > in an ObjectiveC++ program by guessing the wrong language? I suppose this > > just tries to do a best effort? > > I don't see any obvious ways to get it wrong, but can't rule it out entirely. > The way this change is implemented, there should be no way to have a > regression – everything that used to work should still work. There could be > some case out there where, in objc++, the wrong choice is made between > `this`/`self`, but that would result in the same "variable not found" message > you'd get today in that case (since the implementation should have no > regressions). Sorry I didn't realize this was existing code! While there might be an opportunity the improve the existing implementation, that is orthogonal to this change. Carry on. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145276/new/ https://reviews.llvm.org/D145276 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
