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.

Sign up for GitHub

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

Jump to bottom

refactor: Switch to trailing return types in core #599

Merged
merged 2 commits into from
Jun 27, 2022
Merged

refactor: Switch to trailing return types in core #599

merged 2 commits into from
Jun 27, 2022

Conversation

mloskot
Copy link
Member

@mloskot mloskot commented Apr 25, 2021 •
edited
Loading

Description

  • Trailing return types everywhere
  • Optionally, return type deduction where sensible (simple and short functions)
  • Apply // hack for clang-format, see Introduce official clang-format #596 (comment)
  • Extensions, especially the large IO are skipped, left for future

References

Tasklist

  • Ensure all CI builds pass
  • Review and approve - a thorough review is not required

@mloskot mloskot added cat/refactoring Any nonfunctional changes status/work-in-progress Do NOT merge yet until this label has been removed! labels Apr 25, 2021
@mloskot mloskot marked this pull request as draft April 25, 2021 18:04
@mloskot mloskot added this to the Boost 1.77+ milestone Apr 25, 2021
@codecov
Copy link

codecov bot commented Apr 25, 2021 •
edited
Loading

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
Copy link
Member

lpranam commented Apr 26, 2021

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

@lpranam
Copy link
Member

lpranam commented Apr 26, 2021 •
edited
Loading

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
Copy link
Member Author

mloskot commented Apr 26, 2021

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
Copy link
Member

lpranam commented Apr 26, 2021

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
Copy link
Member Author

mloskot commented Apr 26, 2021 •
edited
Loading

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 #599 (comment) I explained that this PR #599 will eventually become a single commit PR. What else do you mean to suggest?

UPDATE: Commits squashed ;)

@mloskot mloskot force-pushed the ml/refactor-with-trailing-return branch from c115a66 to 3caa9ae Compare April 26, 2021 09:00
@lpranam
Copy link
Member

lpranam commented Apr 26, 2021

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

@mloskot
Copy link
Member Author

mloskot commented Apr 26, 2021 •
edited
Loading

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
Copy link
Member

lpranam commented Apr 26, 2021

yeah makes sense, let keep them separate.

@mloskot mloskot force-pushed the ml/refactor-with-trailing-return branch 2 times, most recently from 46b3996 to a0b95e7 Compare May 6, 2021 21:04
@mloskot
Refactor to trailing return types
a0b95e7 
- Trailing return types everywhere
- Optionally, return type deduction where sensible (simple and short functions)

This is related to introduction of common .clang-format,
see boostorg#596 (comment)
@mloskot mloskot changed the title Refactor to trailing return types Refactor to trailing return types in core May 6, 2021
@mloskot mloskot marked this pull request as ready for review May 6, 2021 21:08
@mloskot mloskot added core boost/gil core/io boost/gil/io and removed status/work-in-progress Do NOT merge yet until this label has been removed! labels May 6, 2021
@mloskot
Copy link
Member Author

mloskot commented May 6, 2021 •
edited
Loading

@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
Copy link
Member

lpranam commented May 7, 2021

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

@mloskot
Copy link
Member Author

mloskot commented May 7, 2021 •
edited
Loading

@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 😛

@mloskot mloskot added the ext/image_processing boost/gil/extension/image_processing label Jun 27, 2022
@mloskot mloskot merged commit fe63aa2 into boostorg:develop Jun 27, 2022
@mloskot mloskot deleted the ml/refactor-with-trailing-return branch June 27, 2022 16:01
@mloskot mloskot changed the title Refactor to trailing return types in core refactor: Switch to trailing return types in core Jun 27, 2022
mloskot added a commit that referenced this pull request Jun 29, 2022
@mloskot
Merge branch 'develop'
bba6d62 
* develop:
  test: Add more basic cases for image class (#423)
  test: Add virtual_2d_locator fixture; is_2d_traversable test case
  test: Check more properties of indexed_image_view from extension/toolbox
  test: Add basic is_1d_traversable cases for image_view
  chore: Update CMakeSettings.json sample [ci skip]
  chore: Update CMake to use latest cmake-conan/0.18.1 [ci skip]
  Add pmr image typedefs (#529)
  test: Add test cases for image with empty dimensions (#702)
  test: Case test_constructor_from_view was not called
  fix: Memory leak in image class for empty dimensions (#649)
  docs: Bump C++11 to C++14 as current required (#700)
  ci: Remove C++11 build jobs after C++14 switch (#698)
  build: Fix CMake source file extensions must be explicit
  refactor: Switch to trailing return types (#599)
  build: Bump Boost required by CMake from 1.72 to 1.80
  build: Update CMAKE_CXX_STANDARD from 11 to 14
@mloskot mloskot mentioned this pull request Jul 5, 2022
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cat/refactoring Any nonfunctional changes core/io boost/gil/io core boost/gil ext/image_processing boost/gil/extension/image_processing
Projects
None yet
Milestone
Boost 1.80
Development

Successfully merging this pull request may close these issues.
2 participants
@mloskot @lpranam