Add a method which checks if an expression could be turned into a function call

I want Clang to issue a fixit every time I write 'method_name' where I meant 'method_name()'. As a first step, provide a general-purpose method to check whether an expression looks like something that could be turned into a function call, e.g. a DeclRefExpr which refers to a function, or an expression of function pointer type.

[matthewbg, 2011-04-22 22:13:22]

Still getting the hang of this whole Rietveld thing... anyway, patch
does what it says in the subject, please take a look.

On Fri, Apr 22, 2011 at 15:11,  <matthewbg@google.com> wrote:
> http://codereview.appspot.com/4439065/
>

[chandlerc, 2011-04-22 22:24:44]

Just a few stylistic comments. Generally looks fine to me. I'll be glad to see
the notes added back at some point. =D

http://codereview.appspot.com/4439065/diff/1/include/clang/Sema/Sema.h
File include/clang/Sema/Sema.h (right):

http://codereview.appspot.com/4439065/diff/1/include/clang/Sema/Sema.h#newcod...
include/clang/Sema/Sema.h:1940: bool CanCall(const Expr* E, QualType&
ZeroArgCallReturnTy,
I think a bit more context in the name would help when this is stuffed into the
Sema interface of dhoom...

IsExprCallable?
IsExprCallableFunction?

http://codereview.appspot.com/4439065/diff/1/lib/Sema/Sema.cpp
File lib/Sema/Sema.cpp (right):

http://codereview.appspot.com/4439065/diff/1/lib/Sema/Sema.cpp#newcode757
lib/Sema/Sema.cpp:757: /// Use this when trying to recover from an error that
could have been due to
"... that could have been due to the programmer writing ..."

to

"... where the programmer may have written ..."

http://codereview.appspot.com/4439065/diff/1/lib/Sema/Sema.cpp#newcode767
lib/Sema/Sema.cpp:767: bool Sema::CanCall(const Expr* E, QualType&
ZeroArgCallReturnTy,
Can E be NULL? If not, maybe use a const & here?

http://codereview.appspot.com/4439065/diff/1/lib/Sema/SemaExpr.cpp
File lib/Sema/SemaExpr.cpp (right):

http://codereview.appspot.com/4439065/diff/1/lib/Sema/SemaExpr.cpp#newcode4340
lib/Sema/SemaExpr.cpp:4340: UnresolvedSet<4> AllOverloads;
Maybe an inline size of 1 instead of 4? That seems to be the case you're
optimizing for anyways... but then again, we've already hit an error.

http://codereview.appspot.com/4439065/diff/1/lib/Sema/SemaExpr.cpp#newcode4342
lib/Sema/SemaExpr.cpp:4342: if (ZeroArgCallTy.isNull()) {
No need for an explicit isNull(), conversion to bool works here.

[matthewbg, 2011-04-25 17:55:55]

PTAL

http://codereview.appspot.com/4439065/diff/1/include/clang/Sema/Sema.h
File include/clang/Sema/Sema.h (right):

http://codereview.appspot.com/4439065/diff/1/include/clang/Sema/Sema.h#newcod...
include/clang/Sema/Sema.h:1940: bool CanCall(const Expr* E, QualType&
ZeroArgCallReturnTy,
On 2011/04/22 22:24:44, chandlerc wrote:
> I think a bit more context in the name would help when this is stuffed into
the
> Sema interface of dhoom...
> 
> IsExprCallable?

Good call. I had something long and unwieldy in an early version,
"IsExprCallable" is a nice medium.

Also, is there a better place within this file to put the declaration (likewise
for the definition)?

http://codereview.appspot.com/4439065/diff/1/lib/Sema/Sema.cpp
File lib/Sema/Sema.cpp (right):

http://codereview.appspot.com/4439065/diff/1/lib/Sema/Sema.cpp#newcode757
lib/Sema/Sema.cpp:757: /// Use this when trying to recover from an error that
could have been due to
On 2011/04/22 22:24:44, chandlerc wrote:
> "... that could have been due to the programmer writing ..."
> 
> to
> 
> "... where the programmer may have written ..."

Done.

http://codereview.appspot.com/4439065/diff/1/lib/Sema/Sema.cpp#newcode767
lib/Sema/Sema.cpp:767: bool Sema::CanCall(const Expr* E, QualType&
ZeroArgCallReturnTy,
On 2011/04/22 22:24:44, chandlerc wrote:
> Can E be NULL? If not, maybe use a const & here?

Done.

http://codereview.appspot.com/4439065/diff/1/lib/Sema/SemaExpr.cpp
File lib/Sema/SemaExpr.cpp (right):

http://codereview.appspot.com/4439065/diff/1/lib/Sema/SemaExpr.cpp#newcode4340
lib/Sema/SemaExpr.cpp:4340: UnresolvedSet<4> AllOverloads;
On 2011/04/22 22:24:44, chandlerc wrote:
> Maybe an inline size of 1 instead of 4? That seems to be the case you're
> optimizing for anyways... but then again, we've already hit an error.

This made more sense when I was using AllOverloads for a greater purpose than
asking if there was >1 overload, i.e. printing "did you mean this overload?"
notes. I originally ripped that code out because it was complicated and ugly.
I've re-added it in slightly less ugly form, but there's probably still room for
improvement.

http://codereview.appspot.com/4439065/diff/1/lib/Sema/SemaExpr.cpp#newcode4342
lib/Sema/SemaExpr.cpp:4342: if (ZeroArgCallTy.isNull()) {
On 2011/04/22 22:24:44, chandlerc wrote:
> No need for an explicit isNull(), conversion to bool works here.

Er, compiler disagrees with you.

[matthewbg, 2011-04-28 04:01:21]

PTAL for reals. I factored out the overload note printing, so the boilerplate in
SemaExpr.cpp is down to a pretty reasonable level.

[chandlerc, 2011-04-28 04:20:31]

Some more comments now...

http://codereview.appspot.com/4439065/diff/9001/include/clang/Sema/Sema.h
File include/clang/Sema/Sema.h (right):

http://codereview.appspot.com/4439065/diff/9001/include/clang/Sema/Sema.h#new...
include/clang/Sema/Sema.h:2072: bool IsExprCallable(const Expr& E, QualType&
ZeroArgCallReturnTy,
The style guide actually says to use isExprCallable and printOverloads. For the
latter, consistency with this file would indicate using 'PrintOverloads', but it
should definitely be isExprCallable.

Also in Clang & and * attach to the name not the type.

http://codereview.appspot.com/4439065/diff/9001/lib/Sema/Sema.cpp
File lib/Sema/Sema.cpp (right):

http://codereview.appspot.com/4439065/diff/9001/lib/Sema/Sema.cpp#newcode776
lib/Sema/Sema.cpp:776: ///  call; otherwise, it is set to an empty QualType.
I'm rather surprised we don't instead set a pointer to the FunctionDecl of the
particular (possibly overloaded) function called... Do the callers really only
need the return type?

http://codereview.appspot.com/4439065/diff/9001/lib/Sema/Sema.cpp#newcode813
lib/Sema/Sema.cpp:813: }
Feel free to omit curlies in places like this...

http://codereview.appspot.com/4439065/diff/9001/lib/Sema/Sema.cpp#newcode820
lib/Sema/Sema.cpp:820: // if we actually have an overloaded bound member.
Should this be a fixme? It would seem pretty easy to check ExprTy eagerly above?

http://codereview.appspot.com/4439065/diff/9001/lib/Sema/Sema.cpp#newcode849
lib/Sema/Sema.cpp:849: void Sema::PrintOverloads(const UnresolvedSetImpl&
Overloads,
I kinda want this to mention "notes". Maybe PrintOverloadNotes?

http://codereview.appspot.com/4439065/diff/9001/lib/Sema/Sema.cpp#newcode850
lib/Sema/Sema.cpp:850: const SourceLocation& FinalNoteLoc) {
SourceLocations should be passed by value.

http://codereview.appspot.com/4439065/diff/9001/lib/Sema/Sema.cpp#newcode857
lib/Sema/Sema.cpp:857: if (ShownOverloads >= 4 &&
I think we need to use the actual parameter here... It doesn't even default to 4
IIRC. It's fine if you want to fix that in a follow-up change, I just think its
important to use the right one if we're going to go this far.

http://codereview.appspot.com/4439065/diff/9001/lib/Sema/SemaExpr.cpp
File lib/Sema/SemaExpr.cpp (right):

http://codereview.appspot.com/4439065/diff/9001/lib/Sema/SemaExpr.cpp#newcode...
lib/Sema/SemaExpr.cpp:4411: ExprResult NewBase =
Do we want to suppress diagnostics when trying to act on the call expr? This
would allow us to not, say, diagnose a call to a private method when the real
error was a typo of a public member.

A FIXME is fine. I can show you sometime how to really easily suppress all
diagnostics here, and even do this *first*, prior to suggesting the parens.

[matthewbg, 2011-04-28 18:56:48]

http://codereview.appspot.com/4439065/diff/9001/include/clang/Sema/Sema.h
File include/clang/Sema/Sema.h (right):

http://codereview.appspot.com/4439065/diff/9001/include/clang/Sema/Sema.h#new...
include/clang/Sema/Sema.h:2072: bool IsExprCallable(const Expr& E, QualType&
ZeroArgCallReturnTy,
On 2011/04/28 04:20:31, chandlerc wrote:
> The style guide actually says to use isExprCallable and printOverloads. For
the
> latter, consistency with this file would indicate using 'PrintOverloads', but
it
> should definitely be isExprCallable.
> 
> Also in Clang & and * attach to the name not the type.

Done.

http://codereview.appspot.com/4439065/diff/9001/lib/Sema/Sema.cpp
File lib/Sema/Sema.cpp (right):

http://codereview.appspot.com/4439065/diff/9001/lib/Sema/Sema.cpp#newcode776
lib/Sema/Sema.cpp:776: ///  call; otherwise, it is set to an empty QualType.
On 2011/04/28 04:20:31, chandlerc wrote:
> I'm rather surprised we don't instead set a pointer to the FunctionDecl of the
> particular (possibly overloaded) function called... Do the callers really only
> need the return type?

Sometimes we don't have a FunctionDecl (L810 and below).

And yes, the only thing the caller needs is the return type, unless you can
think of something clever to do with the FunctionDecl.

http://codereview.appspot.com/4439065/diff/9001/lib/Sema/Sema.cpp#newcode813
lib/Sema/Sema.cpp:813: }
On 2011/04/28 04:20:31, chandlerc wrote:
> Feel free to omit curlies in places like this...

Done.

http://codereview.appspot.com/4439065/diff/9001/lib/Sema/Sema.cpp#newcode820
lib/Sema/Sema.cpp:820: // if we actually have an overloaded bound member.
On 2011/04/28 04:20:31, chandlerc wrote:
> Should this be a fixme? It would seem pretty easy to check ExprTy eagerly
above?

Could you elaborate on this?

http://codereview.appspot.com/4439065/diff/9001/lib/Sema/Sema.cpp#newcode849
lib/Sema/Sema.cpp:849: void Sema::PrintOverloads(const UnresolvedSetImpl&
Overloads,
On 2011/04/28 04:20:31, chandlerc wrote:
> I kinda want this to mention "notes". Maybe PrintOverloadNotes?

Changed to NoteOverloads -- depending on our diagnostic client, we may not
actually be printing anything.

http://codereview.appspot.com/4439065/diff/9001/lib/Sema/Sema.cpp#newcode850
lib/Sema/Sema.cpp:850: const SourceLocation& FinalNoteLoc) {
On 2011/04/28 04:20:31, chandlerc wrote:
> SourceLocations should be passed by value.

Done.

http://codereview.appspot.com/4439065/diff/9001/lib/Sema/Sema.cpp#newcode857
lib/Sema/Sema.cpp:857: if (ShownOverloads >= 4 &&
On 2011/04/28 04:20:31, chandlerc wrote:
> I think we need to use the actual parameter here... It doesn't even default to
4
> IIRC. It's fine if you want to fix that in a follow-up change, I just think
its
> important to use the right one if we're going to go this far.

When I said "Magic number" I wasn't kidding:
http://clang.llvm.org/doxygen/SemaOverload_8cpp_source.html#l07153

http://codereview.appspot.com/4439065/diff/9001/lib/Sema/SemaExpr.cpp
File lib/Sema/SemaExpr.cpp (right):

http://codereview.appspot.com/4439065/diff/9001/lib/Sema/SemaExpr.cpp#newcode...
lib/Sema/SemaExpr.cpp:4411: ExprResult NewBase =
On 2011/04/28 04:20:31, chandlerc wrote:
> Do we want to suppress diagnostics when trying to act on the call expr? This
> would allow us to not, say, diagnose a call to a private method when the real
> error was a typo of a public member.
> 
> A FIXME is fine. I can show you sometime how to really easily suppress all
> diagnostics here, and even do this *first*, prior to suggesting the parens.

FIXME'd

[chandlerc, 2011-05-04 20:21:46]

Submit away whenever.... none of this is important

http://codereview.appspot.com/4439065/diff/13001/lib/Sema/Sema.cpp
File lib/Sema/Sema.cpp (right):

http://codereview.appspot.com/4439065/diff/13001/lib/Sema/Sema.cpp#newcode809
lib/Sema/Sema.cpp:809: if (const PointerType *Ptr =
ExprTy->getAs<PointerType>())
What about references?

http://codereview.appspot.com/4439065/diff/13001/lib/Sema/Sema.cpp#newcode833
lib/Sema/Sema.cpp:833: /// A companion to IsExprCallable. In cases when the name
that the programmer
s/Is/is/

[matthewbg, 2011-05-04 22:16:15]

All done, and committed in r130878. Thanks!

On Wed, May 4, 2011 at 13:21,  <chandlerc@gmail.com> wrote:
> Submit away whenever.... none of this is important
>
>
> http://codereview.appspot.com/4439065/diff/13001/lib/Sema/Sema.cpp
> File lib/Sema/Sema.cpp (right):
>
> http://codereview.appspot.com/4439065/diff/13001/lib/Sema/Sema.cpp#newcode809
> lib/Sema/Sema.cpp:809: if (const PointerType *Ptr =
> ExprTy->getAs<PointerType>())
> What about references?
>
> http://codereview.appspot.com/4439065/diff/13001/lib/Sema/Sema.cpp#newcode833
> lib/Sema/Sema.cpp:833: /// A companion to IsExprCallable. In cases when
> the name that the programmer
> s/Is/is/
>
> http://codereview.appspot.com/4439065/
>