Skip to content

Conversation

@frasercrmck
Copy link
Contributor

@frasercrmck frasercrmck commented Aug 5, 2024

This better reflects how Sema is organized upstream, following recent restructuring. Doing all of it at once (moving all SYCL-related code out of SemaDeclAttr) results in an unwieldy diff, interleaving additions and deletions in an unreadable way. Thus the patch is split up into parts for better reviewability.

Code has been moved to a new cpp file - SemaSYCLAttrDecl.cpp - as SemaSYCL.cpp is already large.

This better reflects how Sema is organized upstream, following recent
restructuring. Doing all of it at once (moving *all* SYCL-related code
out of SemaDeclAttr) results in an unwieldy diff, interleaving additions
and deletions in an unreadable way. Thus the patch is split up into
parts for better reviewability.
@frasercrmck frasercrmck requested a review from a team as a code owner August 5, 2024 16:26
Copy link
Contributor

@elizabethandrews elizabethandrews left a comment

Choose a reason for hiding this comment

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

Thanks for doing this. I believe @premanandrao was/is also taking a look at this. He is OOO till Monday. So if you are alright with waiting for this patch to be merged, I would like him to comment as well.

Copy link
Contributor

@smanna12 smanna12 left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for the improvements.

@frasercrmck
Copy link
Contributor Author

Thanks for doing this. I believe @premanandrao was/is also taking a look at this. He is OOO till Monday. So if you are alright with waiting for this patch to be merged, I would like him to comment as well.

ping, @premanandrao - thanks!

@premanandrao
Copy link
Contributor

Thanks for doing this. I believe @premanandrao was/is also taking a look at this. He is OOO till Monday. So if you are alright with waiting for this patch to be merged, I would like him to comment as well.

ping, @premanandrao - thanks!

Thanks for the ping. @elizabethandrews also pinged me on this change. Thanks!

I like the changes and the split to the new file.

Copy link
Contributor

@premanandrao premanandrao left a comment

Choose a reason for hiding this comment

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

Please resolve the conflicts, but LGTM.

@frasercrmck
Copy link
Contributor Author

Please resolve the conflicts, but LGTM.

Thanks for the review. Yeah I fixed conflicts yesterday and it's already conflicting again. It will be good to flush this PR.

@frasercrmck
Copy link
Contributor Author

@intel/llvm-gatekeepers this is good to merge, thank you!

@martygrant martygrant merged commit cb8e0e5 into intel:sycl Aug 15, 2024
@frasercrmck frasercrmck deleted the sycl-move-semadeclattrs-1 branch August 15, 2024 12:34
frasercrmck added a commit to frasercrmck/llvm that referenced this pull request Aug 15, 2024
sommerlukas pushed a commit that referenced this pull request Aug 19, 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.

6 participants