teemperor accepted this revision. teemperor added a comment. This revision is now accepted and ready to land.
Thanks a lot for fixing this, could you point to D103532 <https://reviews.llvm.org/D103532> as the cause for the regression in the commit message? This LGTM in general, but I think there could still be some obscure cases where this could fail. So the switch statement this code is in (and which detects whether its a reference type) is defined as: clang::QualType parent_qual_type( RemoveWrappingTypes(GetCanonicalQualType(type))); switch (parent_qual_type->getTypeClass()) { `GetCanonicalQualType` should strip all the outer and inner type sugar there is. And `RemoveWrappingTypes` removes *most* of the outer type sugar (which isn't necessary anymore as it should be all gone), but also makes types non-atomic (for simplicity reasons). Meanwhile in our reference-case code we try to remove the outer type sugar (and atomic) via `RemoveWrappingTypes`. And if this doesn't end up in a ReferenceType the cast fails. So IIUC there is still a possibility that we fail to cast here in case `GetCanonicalQualType` removes more outer sugar than `RemoveWrappingTypes`, which I think can only happen for type sugar classes not implemented by `RemoveWrappingTypes` (e.g., a quick grep tells me AttributedType and some template stuff is sugar that we don't handle). So I would propose we do the following: 1. This patch can land now as-is to fix the regression. 2. It would be nice if we could expand the test with a few more variations of sugar. E.g., typedef to a reference of a typedef type and all that stuff. 3. I think we can simplify `RemoveWrappingTypes` to remove all type sugar (not just our small list we maintain there). That should eliminate the obscure cases and we no longer need to maintain a list of type sugar there. ================ Comment at: lldb/test/API/lang/cpp/dereferencing_references/main.cpp:10 + // typedef of a reference + td_int_ref td = i; + ---------------- nit: Could you give this a more unique name such as `td_to_ref_type`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113673/new/ https://reviews.llvm.org/D113673 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
