[SYCL] Remove sycl_ext_oneapi_default_context in preview-breaking mode - #22769
[SYCL] Remove sycl_ext_oneapi_default_context in preview-breaking mode#22769softwaresat wants to merge 4 commits into
Conversation
Robertkq
left a comment
Robertkq
left a comment
There was a problem hiding this comment.
LGTM, good work!
Only note: In this repository, the body of the PR usually becomes the commit message of the Squash & Merge procedure when the PR is eventually merged, as such I think you should try to keep the body of the PR as concise as possible on the changes made and to add comments to the PR for explaining the decisions behind them
| /// Return this platform's default context | ||
| /// | ||
| /// \return the default context | ||
| #ifndef __INTEL_PREVIEW_BREAKING_CHANGES |
There was a problem hiding this comment.
So we don't forget to remove the comment as well.
| /// Return this platform's default context | |
| /// | |
| /// \return the default context | |
| #ifndef __INTEL_PREVIEW_BREAKING_CHANGES | |
| #ifndef __INTEL_PREVIEW_BREAKING_CHANGES | |
| /// Return this platform's default context | |
| /// | |
| /// \return the default context |
There was a problem hiding this comment.
Moved comment inside of ifndef
| This extension has been removed and is no longer supported in {dpcpp}. | ||
| Applications should use the `sycl_khr_default_context` extension instead. |
There was a problem hiding this comment.
Usually we use this wording for removed extensions from the template:
| This extension has been removed and is no longer supported in {dpcpp}. | |
| Applications should use the `sycl_khr_default_context` extension instead. | |
| This extension is no longer implemented in DPC++. This specification is being archived only for historical purposes. The APIs defined in this specification no longer exist and cannot be used. |
Although the API is still implemented actually and will be removed prior to 27.0 (8.0.0). I don't mind moving the spec to removed right now so we don't forget to move it later. @intel/dpcpp-doc-reviewers could you please comment on this?
UPD. see the comment from Greg. I've updated the issue tasks - #22581
There was a problem hiding this comment.
I think it would be better to leave the specification in the "deprecated" directory until it is removed in the default case (i.e. even when -fpreview-breaking-changes is not passed).
Do you think there is a danger that we will forget to do this? Would it help us to remember if we put a comment in the Asciidoc file like:
// __INTEL_PREVIEW_BREAKING_CHANGES: Move this specification to the "removed"
// folder when the feature is removed at the next breaking changes window.
That way we will see the comment when we search for __INTEL_PREVIEW_BREAKING_CHANGES
There was a problem hiding this comment.
Updated wording and added suggested comment
| @@ -1,2 +1,2 @@ | |||
| This extension has been deprecated, but the specification is still available | |||
There was a problem hiding this comment.
I think we just remove the extension file from supported and deprecated directories.
UPD. let's keep it for now. Probably a comment like here would be enough.
There was a problem hiding this comment.
Kept extension file and added comment
| @@ -0,0 +1,32 @@ | |||
| // RUN: %clangxx -fsycl -fpreview-breaking-changes -fsyntax-only -Xclang -verify %s | |||
There was a problem hiding this comment.
I think that might be excessive (I mean having this test). @KseniyaTikhomirova WDYT?
There was a problem hiding this comment.
agree, we don't need this. tests below are enough
5 participants
Removes
sycl_ext_oneapi_default_contextin preview-breaking mode.platform::ext_oneapi_get_default_context()andSYCL_EXT_ONEAPI_DEFAULT_CONTEXTwith__INTEL_PREVIEW_BREAKING_CHANGES.platform::khr_get_default_context()and the supporteddevice::ext_oneapi_get_default_context()API.removed/when the API is removed from normal builds.
Addresses the
sycl_ext_oneapi_default_contextportion of #22581.