[PATCH] D100733: [clang] NFC: change uses of `Expr->getValueKind` into `is?Value`

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

In D100733#2759592 <https://reviews.llvm.org/D100733#2759592>, @aaronpuchert wrote:

> Not sure how to feel about this, the value categories are already hard to grasp for most C++ programmers. To solve the implicit move concerns with a new value category seems like cracking a nut with a sledgehammer. But that's not directly related to this change, we can discuss this when the change is there.

Yeah this is unrelated to this patch, but hear me out here, I agree with all you said about the complexity of value categories, but:

- I still think a new value category is simpler than the two phase overload resolution it replaces :D
- This new value category would (so far as this is only an experiment I am running), only apply to standards older than c++23, where from then on it's just the current categories and the rules as in P2266 <https://reviews.llvm.org/P2266>.
- Even in the old standards it replaces, it does not introduce any drastic changes. This new category is the same as XValues as far as overload resolution is concerned (with the small addition that it binds less preferentially to non-const l-value references), and as far as type deduction goes, it behaves exactly as LValues.

In D100733#2759898 <https://reviews.llvm.org/D100733#2759898>, @aaron.ballman wrote:

> I'm not certain this is a big win in all cases, especially because the standard has one set of terminology and we use another. e.g., the standard makes a distinction between rvalue and prvalue that we don't make, same for lvalue and glvalue. When I see the equality expression, I know that only one category is being tested, but when I see the function call expression, I have to think more about "does `isRValue()` return true for a prvalue and an xvalue, or just a prvalue?"

I understand. But consider that it's not complicated right now:

  bool isLValue() const { return getValueKind() == VK_LValue; }
  bool isRValue() const { return getValueKind() == VK_RValue; }
  bool isXValue() const { return getValueKind() == VK_XValue; }
  bool isGLValue() const { return getValueKind() != VK_RValue; }

They are all testing exactly one enum, except the GLValue one. I understand however that I worked in very small subset of clang, this is as easy to remember as it gets for me, but this is not the same for others with different workloads. I am sure you have way more stuff to remember :)

I get @aaronpuchert 's point about how this only makes it consistent for now. I think we should so something about that and still make it more consistent.

How about another approach: we remove all the helpers that test exactly one enum (X, L and R, leaving only GL).
What do you think?

Another approach would be to document, or make more official, that the helpers that test a category with just one letter are just testing one enum value, and the ones with two or more letters are testing something more complex.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100733/new/

https://reviews.llvm.org/D100733