Skip to main content
Code Review

Return to Answer

added 107 characters in body
Source Link
Incomputable
Incomputable
  • 9.6k
  • 3
  • 33
  • 72

There isn't much to talk about, the code is good.

Drop in ergonomics

The template now disallows usage of std::string directly, as the CharType is not automatically deduced. Some other calls where the string view could be converted to are now prohibited as well (C strings, for example). It might make sense to provide some overloads that take C strings and std strings to facilitate easier use.

Missing traits as type parameter for the string_view

I honestly do not know if anybody uses them, but I'll leave it here for completeness.

Mixing two responsibilities

I believe that histogram calculation deserves separation from this function. Perhaps having one easy to use function that composes from them is great, but not having the building blocks separately might cause problems.

I haven't worked with ranges, but what if it would be possible to actually create another view from the input that is histogram of the original, then just apply the algorithm? This might solve the problem that @Toby is mentioning, perhaps the ignoring functions could be written as part of the pipeline.

This might make it viable to accept any range type that has needed properties (foreshadowing concepts).

There isn't much to talk about, the code is good.

Drop in ergonomics

The template now disallows usage of std::string directly, as the CharType is not automatically deduced. Some other calls where the string view could be converted to are now prohibited as well (C strings, for example). It might make sense to provide some overloads that take C strings and std strings to facilitate easier use.

Missing traits as type parameter for the string_view

I honestly do not know if anybody uses them, but I'll leave it here for completeness.

Mixing two responsibilities

I believe that histogram calculation deserves separation from this function. Perhaps having one easy to use function that composes from them is great, but not having the building blocks separately might cause problems.

I haven't worked with ranges, but what if it would be possible to actually create another view from the input that is histogram of the original, then just apply the algorithm? This might solve the problem that @Toby is mentioning, perhaps the ignoring functions could be written as part of the pipeline.

There isn't much to talk about, the code is good.

Drop in ergonomics

The template now disallows usage of std::string directly, as the CharType is not automatically deduced. Some other calls where the string view could be converted to are now prohibited as well (C strings, for example). It might make sense to provide some overloads that take C strings and std strings to facilitate easier use.

Missing traits as type parameter for the string_view

I honestly do not know if anybody uses them, but I'll leave it here for completeness.

Mixing two responsibilities

I believe that histogram calculation deserves separation from this function. Perhaps having one easy to use function that composes from them is great, but not having the building blocks separately might cause problems.

I haven't worked with ranges, but what if it would be possible to actually create another view from the input that is histogram of the original, then just apply the algorithm? This might solve the problem that @Toby is mentioning, perhaps the ignoring functions could be written as part of the pipeline.

This might make it viable to accept any range type that has needed properties (foreshadowing concepts).

Source Link
Incomputable
Incomputable
  • 9.6k
  • 3
  • 33
  • 72

There isn't much to talk about, the code is good.

Drop in ergonomics

The template now disallows usage of std::string directly, as the CharType is not automatically deduced. Some other calls where the string view could be converted to are now prohibited as well (C strings, for example). It might make sense to provide some overloads that take C strings and std strings to facilitate easier use.

Missing traits as type parameter for the string_view

I honestly do not know if anybody uses them, but I'll leave it here for completeness.

Mixing two responsibilities

I believe that histogram calculation deserves separation from this function. Perhaps having one easy to use function that composes from them is great, but not having the building blocks separately might cause problems.

I haven't worked with ranges, but what if it would be possible to actually create another view from the input that is histogram of the original, then just apply the algorithm? This might solve the problem that @Toby is mentioning, perhaps the ignoring functions could be written as part of the pipeline.