Skip to content

[clang][dataflow] Fix result object location for builtin <=>. #88726

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 16, 2024
Merged

[clang][dataflow] Fix result object location for builtin <=>. #88726

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions clang/lib/Analysis/FlowSensitive/DataflowEnvironment.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,6 +508,11 @@ class ResultObjectVisitor : public RecursiveASTVisitor<ResultObjectVisitor> {
isa<CXXStdInitializerListExpr>(E)) {
return;
}
if (auto *Op = dyn_cast<BinaryOperator>(E);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aside: Might lines 506 through 553 be better expressed as a switch on E->getStmtClass()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure.

  • The cases involving a dyn_cast need a cast anyway, as they access properties of the cast class. So if we introduced a switch, we would need a case and a cast.

  • This leaves the isa<> cases above. IIUC, isa<> essentially compiles down to a test on E->getStmtClass(). So in terms of performance, replacing the isa<> calls above with a switch statement shouldn't make much of a difference. I think the isa<> version is more readable though.

So in all, I don't see a clear advantage, and I would prefer to keep this as-is.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the isa calls were what we really jumped out since they amount to N calls and tests on getStmtClass rather than one and a jump. But, readability wins over performance here.

But, I do wonder about readability win from converting to a switch because it will directly express which cases are covered in this function, rather than leaving it implicit in a series of ifs. FWIW.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the isa calls were what we really jumped out since they amount to N calls and tests on getStmtClass rather than one and a jump. But, readability wins over performance here.

I see what you mean -- yes, the switch statement would likely perform better here, but I agree that readability is more important than this micro-optimization.

But, I do wonder about readability win from converting to a switch because it will directly express which cases are covered in this function, rather than leaving it implicit in a series of ifs. FWIW.

I still have my doubts whether this will really be more readable, but I'll explore this. I'd prefer to do this as a separate PR though, as I'd like to keep this PR focused on the fix I'm making here. (If I convert the function to a switch statement in this PR, those code changes would drown out the few lines of actual behavioral change I'm making.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #88865 for the PR that refactors this into a switch statement -- let's continue discussion there.

Op && Op->getOpcode() == BO_Cmp) {
// Builtin `<=>` returns a `std::strong_ordering` object.
return;
}

if (auto *InitList = dyn_cast<InitListExpr>(E)) {
if (!InitList->isSemanticForm())
Expand Down
52 changes: 52 additions & 0 deletions clang/unittests/Analysis/FlowSensitive/TransferTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3098,6 +3098,58 @@ TEST(TransferTest, ResultObjectLocationForCXXOperatorCallExpr) {
});
}

// Check that the `std::strong_ordering` object returned by builtin `<=>` has a
// correctly modeled result object location.
TEST(TransferTest, ResultObjectLocationForBuiltinSpaceshipOperator) {
std::string Code = R"(
namespace std {
// This is the minimal definition required to get
// `Sema::CheckComparisonCategoryType()` to accept this fake.
struct strong_ordering {
enum class ordering { less, equal, greater };
ordering o;
static const strong_ordering less;
static const strong_ordering equivalent;
static const strong_ordering equal;
static const strong_ordering greater;
};

inline constexpr strong_ordering strong_ordering::less =
{ strong_ordering::ordering::less };
inline constexpr strong_ordering strong_ordering::equal =
{ strong_ordering::ordering::equal };
inline constexpr strong_ordering strong_ordering::equivalent =
{ strong_ordering::ordering::equal };
inline constexpr strong_ordering strong_ordering::greater =
{ strong_ordering::ordering::greater };
}
void target(int i, int j) {
auto ordering = i <=> j;
// [[p]]
}
)";
using ast_matchers::binaryOperator;
using ast_matchers::hasOperatorName;
using ast_matchers::match;
using ast_matchers::selectFirst;
using ast_matchers::traverse;
runDataflow(
Code,
[](const llvm::StringMap<DataflowAnalysisState<NoopLattice>> &Results,
ASTContext &ASTCtx) {
const Environment &Env = getEnvironmentAtAnnotation(Results, "p");

auto *Spaceship = selectFirst<BinaryOperator>(
"op",
match(binaryOperator(hasOperatorName("<=>")).bind("op"), ASTCtx));

EXPECT_EQ(
&Env.getResultObjectLocation(*Spaceship),
&getLocForDecl<RecordStorageLocation>(ASTCtx, Env, "ordering"));
},
LangStandard::lang_cxx20);
}

TEST(TransferTest, ResultObjectLocationForStdInitializerListExpr) {
std::string Code = R"(
namespace std {
Expand Down