Skip to content

Conversation

augusto2112
Copy link
Contributor

No description provided.

@augusto2112
Copy link
Contributor Author

@adrian-prantl there are two separate commits here which might be easier to review separately. This depends on #71249

@augusto2112
Copy link
Contributor Author

@swift-ci smoke test

@augusto2112
Copy link
Contributor Author

@swift-ci smoke test

Copy link
Contributor

Choose a reason for hiding this comment

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

std::optional::value_or (llvm::optional also has such a function)

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this intentionally change the behavior for BuiltinRawPointer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I changed it because all of them seem artificial types to me (builtins not visible in user source code). What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Commit #1 doesn't have any tests, but we should at least be able to test the special types?

Copy link
Contributor

Choose a reason for hiding this comment

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

When does createStructType() return a nullptr?

Copy link
Contributor Author

@augusto2112 augusto2112 Feb 3, 2024

Choose a reason for hiding this comment

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

It shouldn't, this is just protecting against UB in case createStructType's implementation is ever changed to return a nullptr.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be clearer to assert(DIType) here. It should catch it if someone changes the implementation and makes the intention clearer. Or the function could just return a reference..

Copy link
Contributor

Choose a reason for hiding this comment

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

What does it mean for this to fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getOrCreateType is quite complex so I don't know if succeeds 100% of the time, this is just to guarantee we don't dereference a null ptr. I'll add an assert to document the fact that we don't expect this to fail (I'll do the same for the one you commented above as well).

Copy link
Contributor

Choose a reason for hiding this comment

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

The LLVM coding style has a really weird aversion to using references. I think it would be fine to assert and not check here.

Copy link
Contributor

@adrian-prantl adrian-prantl left a comment

Choose a reason for hiding this comment

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

Looks mostly good, but both commits are missing tests.

@augusto2112
Copy link
Contributor Author

@swift-ci smoke test

@augusto2112
Copy link
Contributor Author

@swift-ci smoke test

@augusto2112
Copy link
Contributor Author

@swift-ci test Windows

@augusto2112 augusto2112 merged commit dc959a4 into swiftlang:main Feb 7, 2024
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.

2 participants