aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM, though please add a release note for the fix. If you need someone to land on your behalf, let us know what name and email address you'd like used for patch attribution.
================ Comment at: clang/lib/AST/ExprConstant.cpp:7673 + Info.FFDiag(Callee, diag::note_constexpr_null_callee) + << const_cast<Expr *>(Callee); + return false; ---------------- shafik wrote: > hazohelet wrote: > > tbaeder wrote: > > > Is the `const_cast` really necessary? > > > Is the `const_cast` really necessary? > > Without `const_cast`, it did not compile. > > I searched the existing codebase to find this line > > https://github.com/llvm/llvm-project/blob/151d3b607e1e3256ed901e02b48b92e79a77021d/clang/lib/Sema/SemaConcept.cpp#L300 > > and I did the same here. > > > Yeah, we do seem to `const_cast` on `const Expr*` all over and in some places > it is obviously harmless but others it is far from clear, at least on a > cursory examination. Clang has historically had very poor const-correctness, and trying to retrofit it is always an exercise in avoiding viral changes. So new interfaces tend to be more likely to be const-correct, while older ones (like the diagnostics engine) tend to not be. The `const_cast<>` looks necessary to me, but if someone wanted to explore fixing up the interface so we can remove the const_casts from multiple places (as a follow-up patch, not as part of this work), that could be fruitful. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145793/new/ https://reviews.llvm.org/D145793 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
