boostorg / gil

Boost.GIL - Generic Image Library | Requires C++14 since Boost 1.80

https://boostorg.github.io/gil

Boost Software License 1.0

179 stars 163 forks source link

refactor: Switch to trailing return types in core #599

Closed mloskot closed 2 years ago

mloskot commented 3 years ago

Description

Trailing return types everywhere
Optionally, return type deduction where sensible (simple and short functions)
Apply // hack for clang-format, see https://github.com/boostorg/gil/pull/596#issuecomment-833941725
Extensions, especially the large IO are skipped, left for future

References

This is related to introduction of common .clang-format, see https://github.com/boostorg/gil/pull/596#issuecomment-822681523

Tasklist

[x] Ensure all CI builds pass
[ ] Review and approve - a thorough review is not required

codecov[bot] commented 3 years ago

Codecov Report

Merging #599 (390e1cc) into develop (95679b6) will increase coverage by 0.11%. The diff coverage is 99.56%.

@@             Coverage Diff             @@
##           develop     #599      +/-   ##
===========================================
+ Coverage    80.75%   80.86%   +0.11%     
===========================================
  Files          116      116              
  Lines         5086     5117      +31     
===========================================
+ Hits          4107     4138      +31     
  Misses         979      979

lpranam commented 3 years ago

btw how did you find and decide which function should change to trailing return type? did you do it manually or some script?

lpranam commented 3 years ago

I think we should make a new branch for all the refactoring for now, freeze merge in develop for a week or so. put all the refactoring change in this branch and then just squash all those and merge in develop. what are your thoughts?

would love to have a single commit for all these changes(just my preference)

mloskot commented 3 years ago

did you do it manually or some script?

Manually

would love to have a single commit for all these changes(just my preference)

Yes, there certainly will be a single commit. We always do "Squash and merge". The multiple commits is just temporarily, for my convenience.

So, I don't think any freezing is necessary. I will plough on with the PR over next days, syncing it with the develop, then we squash ml/refactor-with-trailing-return and merge. Do you see any problem with that approach?

The branch can be updated by anyone, by the way.

lpranam commented 3 years ago

I was hoping for trailing return + refactor in one commit. changing to trailing return type is a kinda reflector too that's why. Otherwise, we can have these separate I don't mind either way.

mloskot commented 3 years ago

I was hoping for trailing return + refactor in one commit. changing to trailing return type is a kinda reflector too that's why.

I'm sorry but I'm lost.

In https://github.com/boostorg/gil/pull/599#issuecomment-826601761 I explained that this PR #599 will eventually become a single commit PR. What else do you mean to suggest?

UPDATE: Commits squashed ;)

lpranam commented 3 years ago

I mean this trailing return type changes + the Big reformat as one commit

mloskot commented 3 years ago

If you don't mind, I'd prefer to keep them separate. They both fall into the refactoring category, but the C++ syntax change is different from style change.

lpranam commented 3 years ago

yeah makes sense, let keep them separate.

mloskot commented 3 years ago

@lpranam This is finished and if the CI-s get green, then I'm going to merge it. I don't think there is need to actually run the line by line review, unless you think otherwise.

lpranam commented 3 years ago

@mloskot I am happy to not review this line by line 😉

mloskot commented 3 years ago

@lpranam

I am happy to not review this line by line 😉

https://www.youtube.com/watch?v=EN90RWb9f9M&t=23s

I just wanted to make it clear in case you wonder what I may expect to happen about it here 😛