Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Remove mixed use of tabs and spaces #936

Conversation

frasercrmck
Copy link
Contributor

Tabs can been replaced with spaces as spaces were predominant. Formatting was done with clang-format on functions containing tabs - rather than the whole file - to try and reduce the diff.

The mixed tabs and spaces can cause issues in readability, for instance misleading indentation (some compilers pick up on this).

@frasercrmck frasercrmck requested a review from a team as a code owner September 10, 2024 10:35
Tabs can been replaced with spaces as spaces were predominant.
Formatting was done with clang-format on functions containing tabs -
rather than the whole file - to try and reduce the diff.
@psalz
Copy link
Member

psalz commented Sep 10, 2024

AFAIK this file was copied from the corresponding file in the OpenCL CTS, so I wonder if instead of reformatting it ourselves, we should just pull in the newer upstream version.

@frasercrmck
Copy link
Contributor Author

AFAIK this file was copied from the corresponding file in the OpenCL CTS, so I wonder if instead of reformatting it ourselves, we should just pull in the newer upstream version.

Yeah maybe that's a better way of doing it. Though we'd either have to examine the differences between the two files and/or trust that our testing is robust enough to catch the inevitable minor differences in reference maths that'll come with taht.

Copy link
Member

@keryell keryell left a comment

Choose a reason for hiding this comment

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

Yes, it sounds good to first fix this upstream and then check for the differences.

@frasercrmck frasercrmck deleted the fix-mix-tabs-and-spaces branch September 17, 2024 07:47
@frasercrmck
Copy link
Contributor Author

That said, since upstream's version has already been formatted, I wonder if it would make sense for us to format our copy first before pulling, so that any differences between the two files would become more apparent, especially for reviewers?

@frasercrmck frasercrmck restored the fix-mix-tabs-and-spaces branch September 17, 2024 08:06
@psalz
Copy link
Member

psalz commented Sep 17, 2024

Yes, although the OpenCL CTS uses a different clang-format configuration. I'm actually not sure where this file was taken from originally, because the version in the first public commit of the OpenCL CTS is already different (mostly whitespace changes as far as I can tell). Maybe we should start by updating to that version, and then see if we need any of the newer changes made since then.

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.

3 participants