Page MenuHomePhabricator

[clang] NFC: refactor usage of getDecltypeForParenthesizedExpr
Needs ReviewPublic

Authored by mizvekov on Sat, Apr 17, 6:51 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

This cleanup patch refactors a bunch of duplicates of
getDecltypeForParenthesizedExpr into a common implementation.

Signed-off-by: Matheus Izvekov <mizvekov@gmail.com>

Diff Detail

Event Timeline

mizvekov created this revision.Sat, Apr 17, 6:51 PM
mizvekov requested review of this revision.Sat, Apr 17, 6:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptSat, Apr 17, 6:51 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
mizvekov edited the summary of this revision. (Show Details)Sat, Apr 17, 6:54 PM

It seems that using is*Value and the introduction of getDecltypeForParenthesizedExpr could be two separate changes.

clang/lib/Sema/SemaExprCXX.cpp
5845

The quote doesn't reference parenthesized expressions, isn't this just coincidentally the same thing?

clang/lib/StaticAnalyzer/Core/CallEvent.cpp
72

This seems also more of a coincidence. There is no parenthesized expression, we're just trying to figure out a function return type.

(Ok, it's not a pure coincidence, the decltype is probably chosen to match that type.)

mizvekov added inline comments.Sat, Apr 17, 7:35 PM
clang/lib/Sema/SemaExprCXX.cpp
5845

It's fundamentally the same thing. The getDecltypeForParenthesizedExpr name is what I tried to keep, I don't have better ideas there.

clang/lib/StaticAnalyzer/Core/CallEvent.cpp
72

Yes, not a coincidence, still fundamentally the same thing.

mizvekov updated this revision to Diff 338393.Sun, Apr 18, 1:22 PM

Split 'getValueKind' cleanup into a separate patch.

mizvekov retitled this revision from [clang] NFC: refactor usage of getDecltypeForParenthesizedExpr and getValueKind to [clang] NFC: refactor usage of getDecltypeForParenthesizedExpr.Sun, Apr 18, 1:22 PM
mizvekov edited the summary of this revision. (Show Details)

Again, I'm not sure if it helps to use getDecltypeForParenthesizedExpr where we don't actually have the decltype of a parenthesized expression.

It's probably not entirely coincidental, but things aren't defined this way.

clang/lib/Sema/SemaExprCXX.cpp
5845

What this is doing is pointwise equal to getDecltypeForParenthesizedExpr, but there is no parenthesized expression, and no decltype. There is a quote from the standard that defines this separately (by now this seems to be expr.cond#4), and there are some differences especially in the prvalue case. So I'm not sure this helps.

Again, I'm not sure if it helps to use getDecltypeForParenthesizedExpr where we don't actually have the decltype of a parenthesized expression.

It's probably not entirely coincidental, but things aren't defined this way.

The current situation with getDecltypeForParenthesizedExpr is that it already is used in some places where there is no decltype of a parenthesized expression either.

This function is a bit new, I introduced it in D98160. I think the mnemonic is OK, but this is the best I could come up at the time. If you have better suggestions, I am all for it.
In the above patch, this is introduced to help out with the type for the expression in a compound requires (http://eel.is/c++draft/expr.prim.req.compound#1.3.2).
But then it is also used in the implementation of decltype itself, where it kicks in even for non-parenthesized expressions (if the expression is not otherwise a special case, as defined in https://eel.is/c++draft/dcl.type.decltype#1).
I think the standard just missed giving a name to this pattern, and it otherwise IMHO misses the mark by using something more complex to explain something simpler.

I think it's no coincidence that for the patch I am working on, I had to change all of these copies in exactly the same way :)

clang/lib/Sema/SemaExprCXX.cpp
5845

Here for XValue and and LValue, the rules are exactly the same as https://eel.is/c++draft/dcl.type.decltype#1.
And the original code here even already special cases it...

I am sure there is some way we can agree that we should not repeat the code just because the standard did not bother to give a name to this part of the rules... And again I think it is no coincidence that it makes sense to perform the same changes in all these cases.