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

parsercsv: Minor refactor to prefer standard algorithms and datatypes #10871

Conversation

tcoyvwac
Copy link
Contributor

@tcoyvwac tcoyvwac commented Sep 6, 2022

As titled. 🎉

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

thanks. a couple minor comments

src/library/parsercsv.cpp Outdated Show resolved Hide resolved
src/library/parsercsv.cpp Outdated Show resolved Hide resolved
Comment on lines 61 to -66
for (int row = 1; row < tokens.size(); ++row) {
if (locationColumnIndex < tokens[row].size()) {
locations.append(tokens[row][locationColumnIndex]);
Copy link
Member

Choose a reason for hiding this comment

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

would you mind making this range-based for as well?

@tcoyvwac tcoyvwac force-pushed the fix/refactor/prefer-standard-library-datatypes-parsercsv branch 2 times, most recently from 09ccbe6 to 5429ad4 Compare September 6, 2022 20:56
Comment on lines 63 to 70
std::for_each(std::next(std::begin(tokens)),
std::end(tokens),
[&](const auto& token) {
if (locationColumnIndex < token.size()) {
locations.append(token[static_cast<int>(
*locationColumnIndex)]);
}
});
Copy link
Member

@Swiftb0y Swiftb0y Sep 6, 2022

Choose a reason for hiding this comment

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

Mhmm. the code has just gotten more complex. I didn't anticipate that. My fault. Would you mind reverting the change again. Sorry.

Copy link
Contributor Author

@tcoyvwac tcoyvwac Sep 6, 2022

Choose a reason for hiding this comment

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

Yeah 😿 , the Mixxx project does not use the range-v3 library yet (hint-hint! 😉)... Then the code would be as simple as:

for(const auto& token: ranges::views::tail(tokens)) {
  if (locationColumnIndex < token.size()) {
      locations.append(token[static_cast<int>(
              *locationColumnIndex)]);
  }
}

and many more nice things... 🥲 (range-v3 stuff will arrive into the standard in C++23)
I agree with you jointly that a range-based approach should be the default writing-style / default way to go.

Will revert back to the previous raw-for-loop form.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah... but expect to wait a bit for that... It was already a pain to switch to C++20...
You're welcome to add the ranges-v3 library as a sort of polyfill to mixxx if you'd like. Do you have any good resources to recommend to learn about ranges? I don't know any mixxx maintainer that knows how to use them (me included).

Copy link
Contributor Author

@tcoyvwac tcoyvwac Sep 6, 2022

Choose a reason for hiding this comment

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

Interesting question @Swiftb0y, humm, it really depends on one's C++ knowledge.
The tl;dr form is:

  1. Understanding Standard Library algorithms and why they are important (C++03 minimum).
  1. Understanding the concept & reasoning behind boost::range (Basically range-v2).
    (Why is this range-v2? From the History section of boost::range:)
    This version introduced Range Adaptors and Range Algorithms. This version 2 is the result of a merge of all of the RangeEx features into Boost.Range.
  1. Understanding about range-v3

Some bonus stuff:


(Sadly,) This is a quick writeup, so thats all for now! You can always playtest with the concepts about ranges on godbolt.org.

Hope this helps and guides on your ranges-concept journey, ye? 😸
But beware, this programming-paradigm path is quite addicting and could lead you to some "heavier stuff"....! 🙃

Copy link
Member

Choose a reason for hiding this comment

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

Thanks a bunch. I'll definitely check those links out when I find the time.

@tcoyvwac tcoyvwac force-pushed the fix/refactor/prefer-standard-library-datatypes-parsercsv branch from 5429ad4 to b563ed9 Compare September 6, 2022 22:11
@tcoyvwac tcoyvwac force-pushed the fix/refactor/prefer-standard-library-datatypes-parsercsv branch from b563ed9 to 3ae9de6 Compare September 6, 2022 22:14
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

lgtm. thanks

@Swiftb0y Swiftb0y merged commit f3a3e04 into mixxxdj:main Sep 6, 2022
@tcoyvwac
Copy link
Contributor Author

tcoyvwac commented Sep 6, 2022

Thanks @Swiftb0y for the... swift PR review and merge! Very grateful ye? 😸 🎉

@tcoyvwac tcoyvwac deleted the fix/refactor/prefer-standard-library-datatypes-parsercsv branch September 6, 2022 23:39
@ronso0 ronso0 added this to the 2.4.0 milestone Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants