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

More clang tidy #908

Merged
merged 8 commits into from
Aug 23, 2021
Merged

More clang tidy #908

merged 8 commits into from
Aug 23, 2021

Conversation

seanm
Copy link
Contributor

@seanm seanm commented Aug 13, 2021

No description provided.

@@ -65,23 +65,23 @@ main(void)
hid_t string_type;
hid_t file_id;
hsize_t chunk_size = 10;
Particle fill_data[1] = {{"no data", -1, -1, -99.0f, -99.0}}; /* Fill value particle */
Particle fill_data[1] = {{"no data", -1, -1, -99.0F, -99.0}}; /* Fill value particle */
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't we just change all these to lower case "f" earlier this year?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know. I don't follow HDF5 closely. The clang-tidy docs are here but they don't really give a rationale. I think it's mostly for l which can look a lot like 1. I guess the idea is to consistently use uppercase. I can drop this commit, I don't have strong feelings about it for f vs F, but for l vs L I do think uppercase is better.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not entirely sure myself, but I think it was a gcc warning issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just checked the develop branch, and there is a mix of f and F currently.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unsurprising.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So using readability-uppercase-literal-suffix as per this patch would have the advantage of making it uniform, even though F vs f is not as useful as l vs L.

Copy link
Contributor

Choose a reason for hiding this comment

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

We did a quick check and it seems we have no standard for F vs f. We will need to decide so we don't keep churning.

Copy link
Member

Choose a reason for hiding this comment

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

We have no standard, but I'm fine with upper case F being the standard, especially given the l vs L confusion.

@lrknox lrknox merged commit f6c49fe into HDFGroup:develop Aug 23, 2021
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