This is an archive of the discontinued LLVM Phabricator instance.

Add hasRetValue narrowing matcher for returnStmt
AbandonedPublic

Authored by LegalizeAdulthood on Jan 24 2016, 1:25 PM.

Details

Summary

Add hasRetValue narrowing matcher for returnStmt so that you can match on the expression returned by a return stmt (or lack thereof).

Currently you can only match the type of a return value on a functionDecl with returns.

An example:

clanger> cat /tmp/a.cpp
void g() {
  return;
}

void f() {
  return (void) g();
}

int h() {
  return 1;
}

int j() {
  return h();
}

bool k() {
  return true;
}
~/dev/build
clanger> bin/clang-query /tmp/a.cpp -- -std=c++11
clang-query> match functionDecl(returns(voidType()))

Match #1:

/tmp/a.cpp:1:1: note: "root" binds here
void g() {
^~~~~~~~~~

Match #2:

/tmp/a.cpp:5:1: note: "root" binds here
void f() {
^~~~~~~~~~
2 matches.
clang-query> match returnStmt(unless(hasRetValue(expr())))

Match #1:

/tmp/a.cpp:2:3: note: "root" binds here
  return;
  ^~~~~~
1 match.
clang-query> ^D

Diff Detail

Event Timeline

LegalizeAdulthood retitled this revision from to Add hasRetValue narrowing matcher for returnStmt.
LegalizeAdulthood updated this object.
LegalizeAdulthood added a reviewer: klimek.
LegalizeAdulthood added a subscriber: cfe-commits.

Run clang-format

aaron.ballman added a subscriber: aaron.ballman.

I get the same behavior from the existing has() matcher -- how is this meant to differ?

clang-query> match returnStmt(unless(has(expr())))

Match #1:

E:\Desktop\test4.cpp:2:3: note: "root" binds here

return;
^~~~~~

clang-query> match returnStmt(has(expr(cxxBoolLiteral())))

Match #1:

E:\Desktop\test4.cpp:18:3: note: "root" binds here

return true;
^~~~~~~~~~~

1 match.

I get the same behavior from the existing has() matcher -- how is this meant to differ?

Hmm! Good question. I suppose it doesn't do anything different.

I was thinking how to write such matching expressions and I looked through the matcher reference and the header file for things related to ReturnStmt and found only returns. It never occurred to me to use has. Not seeing returnStmt mentioned anywhere, it seemed that I couldn't write such matching expressions and needed a new matcher.

So the question remains: is it bad to have a matcher that explicitly models the narrowing for a return statement's return expression when you can achieve the same thing with has?

aaron.ballman edited edge metadata.Jan 25 2016, 6:39 AM

I get the same behavior from the existing has() matcher -- how is this meant to differ?

Hmm! Good question. I suppose it doesn't do anything different.

I was thinking how to write such matching expressions and I looked through the matcher reference and the header file for things related to ReturnStmt and found only returns. It never occurred to me to use has. Not seeing returnStmt mentioned anywhere, it seemed that I couldn't write such matching expressions and needed a new matcher.

Yes, I have definitely run into this line of thinking in the past as well. Specifically, I ran into it with cxxCatchStmt (to determine what kind of type the catch statement will handle).

So the question remains: is it bad to have a matcher that explicitly models the narrowing for a return statement's return expression when you can achieve the same thing with has?

I'm of two minds on this: (1) the status quo is a bit obtuse and so a dedicated matcher might not be a bad thing, (2) AST matchers are a bit expensive due to the template metaprogramming and duplicate functionality is easy to get out of sync. I am leaning towards not duplicating AST matcher functionality because of (2) and think (1) can be corrected with some better documentation for has(), returnStmt(), and perhaps cxxCatchHandler(). The fact that there are > 1 instances where has() works but we might want a dedicated matcher also suggests that we don't want to duplicate. @klimek, what is your stance on the idea?

I'm of two minds on this [...]

I just need a thumbs up/down on this.

Thumbs down, I discard the review.

Thumbs up, you take the patch.

Patch by Richard Thomson.

I'm of two minds on this [...]

I just need a thumbs up/down on this.

Thumbs down, I discard the review.

Thumbs up, you take the patch.

Patch by Richard Thomson.

I think the patch should be discarded. I appreciate the time you put into making the patch, however!

LegalizeAdulthood abandoned this revision.Feb 1 2016, 1:35 PM