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

clang-tidy: use constexpr for unmodified structs #1302

Closed
wants to merge 1 commit into from

Conversation

neheb
Copy link
Collaborator

@neheb neheb commented Sep 16, 2020

Found with cppcoreguidelines-interfaces-global-init

Signed-off-by: Rosen Penev rosenp@gmail.com

Went overboard and converted all extern const to constexpr.

@neheb neheb marked this pull request as draft September 16, 2020 08:30
Copy link
Collaborator

@piponazo piponazo left a comment

Choose a reason for hiding this comment

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

Just for the sake of keeping track about important changes, I would move the 2nd commit to a separate PR. We discussed in the past to do such radical formatting on the whole project and we hesitated about it ... mainly because of the following reasons:

  • It would make more difficult to merge things from 0.27-maintenance to master.
  • It would make more difficult to track the history of changes in files.

It has been almost 2 years since we decided to move on to c++11 in master, and probably we will have the last 0.27 release this year. For me it would be fine to do the clang-format change in master, but let's give the change for other contributors to share their thoughts on this.

@@ -32,7 +32,7 @@ struct TiffTagInfo {
const char* name_;
};

extern const TiffTagInfo tiffTagInfo[] = {
constexpr TiffTagInfo tiffTagInfo[] = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Honestly, I do not understand the benefits of these changes. Could you please give some details about it? 😉

Found with cppcoreguidelines-interfaces-global-init

Signed-off-by: Rosen Penev <rosenp@gmail.com>
@neheb
Copy link
Collaborator Author

neheb commented Sep 17, 2020

Hrm maybe another time.

@neheb neheb closed this Sep 17, 2020
@neheb neheb deleted the constexpr branch September 17, 2020 08:45
@clanmills
Copy link
Collaborator

We discussed in the past to do such radical formatting on the whole project and we hesitated about it ... mainly because of the following reasons:

  • It would make more difficult to merge things from 0.27-maintenance to master.
  • It would make more difficult to track the history of changes in files.

It has been almost 2 years since we decided to move on to c++11 in master, and probably we will have the last 0.27 release this year.

Let's not do this to the code at the moment. We have to decide:

  1. Is 'master' ready to be released as 0.28?
    There are quite a lot of PRs in 0.27-maintenance that should be ported to 'master'.
    I would like to see 0.28 released.
  2. Will we have a 0.27.4 release?

Making a release is always more work that expected. It's discussed in Chapter 13 of my book https://clanmills.com/exiv2/book/#13-9

My priority in 2020 is to get the book finished. I'm willing to one more release of Exiv2 - either 0.27.4 or 0.28. I am willing to do 0.28 in 2021 and that would include merging the PRs from 0.27.3. After that's done, we can reformat all the code.

Realistically 0.28 RC1 could be 2021-03-01, RC2 2021-04-01, and GM 2021-05-01. Ready for the LGM conference in Rennes in May.

I don't know of any security issues in 0.27-maintenance. It's possible that 0.27.4 will never be needed, or it will be 0.27.3+security fixes.

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