Page MenuHomePhabricator

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

Authored by mizvekov on Sun, Apr 18, 1:40 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

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

Diff Detail

Event Timeline

mizvekov created this revision.Sun, Apr 18, 1:40 PM
mizvekov requested review of this revision.Sun, Apr 18, 1:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptSun, Apr 18, 1:40 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

The change seems to be correct, but I'm wondering if x.getValueKind() == VK_*Value doesn't have one advantage over x.is*Value(): it's obvious that this is exclusive with the other values. Especially with isRValue() it might not be so obvious, because Clang doesn't follow the C++11 terminology with this.

But it's admittedly shorter, so I'd be willing to approve this.

clang/lib/Sema/SemaExpr.cpp
5522

There might be a certain benefit to using LHSExp->getValueKind() above when we use it here again: that makes it more obvious what we're trying to achieve in that if. (Namely changing the value category.)

The change seems to be correct, but I'm wondering if x.getValueKind() == VK_*Value doesn't have one advantage over x.is*Value(): it's obvious that this is exclusive with the other values. Especially with isRValue() it might not be so obvious, because Clang doesn't follow the C++11 terminology with this.

But it's admittedly shorter, so I'd be willing to approve this.

This came up in a patch where I am experimenting with a new value category.
The helpers 'help' a lot more when you are changing some of these tests from testing just one category, to testing a combination of categories (like GLValue).

But this is just the easy pickings of the patch, which is still WIP, and I may need in the future for some more drastic change to deal with the code that just stores the kind in a variable and tests that later.

It would be useful if you could do:

VK = Expr->getValueKind();
if (VK.isGLValue()) {

I may need to do something like that later.

clang/lib/Sema/SemaExpr.cpp
5522

Or just making the same kind of change here again:

if (!LHSExp->isRValue())
  OK = OK_VectorComponent;
VK = LHSExp->getValueKind();