[PATCH] D145793: [clang][AST] Improve diagnostic for `nullptr` constexpr function pointer call

[Aaron Ballman via Phabricator via cfe-commits]

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