Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Cleaning up a few places in the code #1784

Merged
merged 6 commits into from
Oct 3, 2024
Merged

Conversation

akukanov
Copy link
Contributor

@akukanov akukanov commented Aug 19, 2024

A couple of changes lurking in my working copy, plus the changes for unused variables from #471 that still apply, and dropping colons before std in the code around.

@akukanov akukanov marked this pull request as draft September 25, 2024 14:20
@akukanov akukanov changed the title Cosmetic cleanup in a few places in the code Cleanup in a few places in the code Oct 1, 2024
Co-authored-by: Dmitriy Sobolev <dmitriy.sobolev@intel.com>
@akukanov akukanov force-pushed the dev/cosmetic-cleanup-akukanov branch from b75b83d to 1af1e4e Compare October 1, 2024 14:15
@akukanov akukanov force-pushed the dev/cosmetic-cleanup-akukanov branch from b9164f1 to c2e9295 Compare October 1, 2024 14:51
@akukanov akukanov marked this pull request as ready for review October 1, 2024 14:53
@akukanov akukanov changed the title Cleanup in a few places in the code Cleaning up a few places in the code Oct 1, 2024
@SergeyKopienko
Copy link
Contributor

@akukanov I think technically this patch is OK.
But is it a good practice to mix some significant changes like remove unused param from __parallel_transform_scan_base together with some unsignificant changes like remove :: in one PR?

@akukanov
Copy link
Contributor Author

akukanov commented Oct 1, 2024

@akukanov I think technically this patch is OK. But is it a good practice to mix some significant changes like remove unused param from __parallel_transform_scan_base together with some unsignificant changes like remove :: in one PR?

We have decided in the past that we will gradually remove :: as we update the code base, Essentially, I removed it from the functions & classes touched by the more significant changes - but yes, it's somewhat wider than only the modified expressions. Since the whole patch is about cleaning up the code without any functionality add or loss, I tend to think it's OK.

Update: it seems the changes in __parallel_copy_if only consist of the colon removal. That's accidental, but I do not think it makes much sense to revert these 4 changed lines.

@akukanov akukanov merged commit 29cd230 into main Oct 3, 2024
21 of 22 checks passed
@akukanov akukanov deleted the dev/cosmetic-cleanup-akukanov branch October 3, 2024 13:34
Copy link
Contributor

@SergeyKopienko SergeyKopienko left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

3 participants