Skip to content

Conversation

@jsji
Copy link
Contributor

@jsji jsji commented May 10, 2024

In different version of glibc , we may get struct instead of class.

To avoid noisy failures, we can remove the keyword check here.

In different version of glibc , we may get `struct` instead of `class`.

To avoid noisy failures, we can remove the keyword check here.
@jsji jsji requested a review from a team as a code owner May 10, 2024 14:43
@jsji jsji requested a review from uditagarwal97 May 10, 2024 14:44
@jsji jsji self-assigned this May 10, 2024
@jsji jsji temporarily deployed to WindowsCILock May 10, 2024 14:55 — with GitHub Actions Inactive
@jsji jsji temporarily deployed to WindowsCILock May 10, 2024 16:14 — with GitHub Actions Inactive
// CHECK-NEXT: 40 | class std::__shared_count<> _M_refcount
// CHECK-NEXT: 40 | _Sp_counted_base<(_Lock_policy)2U> * _M_pi
// CHECK-NEXT: 48 | class std::error_code MErrC
// CHECK-NEXT: std::error_code MErrC
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we instead do something like {{class|struct}} std::error_code... ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can, but I don't think that will improve the testing, eg: we may fail again if there are some other keywords around it due to different env.

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 "48" is kind of important here because we do want to know the offsets and ensure it's unchanged for the purposes of ABI compatibility. I think @uditagarwal97 's initial suggestion is the way to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fine for me for now.

Copy link
Contributor

@uditagarwal97 uditagarwal97 left a comment

Choose a reason for hiding this comment

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

LGTM!

@jsji
Copy link
Contributor Author

jsji commented May 11, 2024

@intel/llvm-gatekeepers Please help to merge this. Thanks.

1 similar comment
@jsji
Copy link
Contributor Author

jsji commented May 13, 2024

@intel/llvm-gatekeepers Please help to merge this. Thanks.

@jsji jsji temporarily deployed to WindowsCILock May 13, 2024 16:34 — with GitHub Actions Inactive
@jsji jsji temporarily deployed to WindowsCILock May 13, 2024 17:17 — with GitHub Actions Inactive
@jsji
Copy link
Contributor Author

jsji commented May 14, 2024

@intel/llvm-gatekeepers Please help to merge this. Thanks.

@aelovikov-intel aelovikov-intel merged commit 5e8085d into sycl May 14, 2024
@aelovikov-intel aelovikov-intel deleted the syclexcep branch May 14, 2024 02:09
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.

4 participants