Skip to content
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.

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

[APValue][BoundsSafety] Migrate away from PointerUnion::{is,get} (NFC) #9834

Open
wants to merge 2 commits into
base: next
Choose a base branch
from

Conversation

rapidsna
Copy link

PointerUnion::{is,get} are deprecated in PointerUnion.h:
// FIXME: Replace the uses of is(), get() and dyn_cast() with
// isa, cast and the llvm::dyn_cast

In this patch, I separated definitions of template functions from the header, in order to avoid nasty instantiation issues seemingly related to header dependency.

rdar://142909627

PointerUnion::{is,get} are deprecated in PointerUnion.h:
    // FIXME: Replace the uses of is(), get() and dyn_cast() with
    //        isa<T>, cast<T> and the llvm::dyn_cast<T>

In this patch, I separated definitions of template functions from
the header, in order to avoid nasty instantiation issues seemingly
related to header dependency.

rdar://142909627
@rapidsna
Copy link
Author

@swift-ci test

Copy link

@cachemeifyoucan cachemeifyoucan left a comment

Choose a reason for hiding this comment

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

Thanks!

@rapidsna
Copy link
Author

@swift-ci test llvm

@vsapsai
Copy link

vsapsai commented Jan 15, 2025

Confirm that can reproduce the linker errors locally.

There are other warnings about PointerUnion deprecation but they aren't in APValue so they are out of scope for this change.

Copy link

@cachemeifyoucan cachemeifyoucan left a comment

Choose a reason for hiding this comment

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

Ah, might need to pull the template into the header since some binaries doesn't setup the dependency to run link APValue.cpp.o.

@rapidsna
Copy link
Author

I can fix the link error. Adding explicit instantiation for is in .cpp should fix that.

@rapidsna
Copy link
Author

@swift-ci test

@rapidsna
Copy link
Author

@swift-ci test llvm

@rapidsna
Copy link
Author

Looks like there are a bunch of unrelated build failures coming from other changes.

@@ -44,6 +44,38 @@ APValue::LValueBase::LValueBase(const ValueDecl *P, unsigned I, unsigned V)
APValue::LValueBase::LValueBase(const Expr *P, unsigned I, unsigned V)
: Ptr(P), Local{I, V} {}

// Separated definitions of these template functions to avoid nasty build issues

Choose a reason for hiding this comment

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

Nit: guard this by // TO_UPSTREAM(BoundsSafety)?

@rapidsna
Copy link
Author

@swift-ci test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants