I wanted to raise this issue here, partly to gain greater visibility
and partly to gauge people's opinions. This commit arose out of
an IR generation bug (which I fixed separately in r139657) where
IR generation was assuming that the semantics of making a call
to a function were the same whether you evaluated the callee
normally or whether you evaluated it by directly taking the
address of the function. Obviously, this is not true in the presence
of explicit casts.
Now, it struck me (and Doug) as pretty suspect for getCalleeDecl()
to ever look through an explicit cast, precisely because that's a
potentially very important change in the call behavior. Ergo, this
change. However, this is clearly a balance between clients who
really don't care about explicit casts (and therefore would prefer
getCalleeDecl() to look through them) and clients who haven't
considered whether their analysis is reasonable in the face of
explicit casts (and therefore would probably prefer
getCalleeDecl() to not look through them).
So consider this an opportunity to register your opinion and maybe
get this changed back.
Why not add a new function (or flag) which allows the programmer to choose whether they look through an explicit cast or not?
I'm not convinced it's useful, and there's value in not having a bunch of subtly different methods.
The right answer is rarely to add a flag and let a programmer control it. This is just pushing off responsibility to the end-user. Even if you do that, you have to argue about what the default is.
No-op explicit cast are ignored?
Not at the moment. Maybe they should be.
I mentioned it only because there seemed to be some ambiguity over which way this function should go. There are only a few options if this is a problem; one of which is to have subtly different functions doing the same thing. But that's not desirable, as you pointed out (and I concur, I just mentioned it as a way to avoid the problem you wrote about (and, no, flags aren't much better)).
My vote is for correctness over convenience – those who care about explicit casts are at a disadvantage here, while those who don't care about them could conceivably "look through" them before calling this function. So I concur with you and Doug.
FTR, I’m really in favor of your change, despite likely caring about a fair amount of code that could be impacted. Specifically, I welcome having to teach code to explicitly look through explicit casts. It ensures that the reader and maintainer also consider in the future this behavior. Silently looking through unexpected AST entities has been cited as a source of confusion for several users we work with when trying to manipulate and understand the AST.