Replace const std::string& with std::string_view

[Lestropie]

Proposed extension of #3212; listing separately for transparency / discussion.

At some point down the code quality rabbit-hole, particularly in seeking getting rid of all char* / char** usage beyond just the C++ CLI code in #2911, I came across @daljit46's use of std::string_view. This is intended to simultaneously:

• Cover string manipulations that could be operating on either C-style char arrays or std::string;
• Be a very explicit read-only view of a string for which data ownership is elsewhere, and therefore a direct substitute for specifically "const std::string&".

The latter is appealing from a code quality perspective. One item that's often on my mind when navigating the code base is the regular use of pass-by-reference arguments to functions: sometimes it's as a read-only reference to avoid data duplication, others it's to enable functions to write back more than one piece of data (which I increasingly don't like; defining a struct to encapsulate all content returned by a function would be preferable). The presence or absence of const isn't ideal for discriminating between these two very different purposes.

As such, given I was already committed to #3212, I decided to have a go at forbidding the use of const std::string &. There's only one remaining set of functions that persist with using const std::string &, and that's only because they're both private functions and the data immediately have to go through a std::istringstream.

There are two main items to which I draw attention:

1. There are a handful of STL items---primarily file I/O I think---that still require a null-terminated character array as input. However the .data() pointer of std::string_view is not guaranteed to be null-terminated, and therefore should not be passed to such functions. Hopefully this shouldn't be a problem if all file manipulations are done through API calls, and those API calls utilise std::string().c_str().

2. Much of the remaining clunkiness of this transition revolves around string concatenation via MR::operator+(). These would be more comprehensively addressed by extending Add {}-style string formatting for logging macros #3125 or a replacement to deal with string concatenation & formatting more generally.

[daljit46]

I think this is a good idea, but there is one specific case where const std::string& is preferable over std::string_view. std::string guarantees a c_str() that null-terminated, which can be useful when interacting with a C API. I'm not sure if we have cases in our codebase where we need this, but it might be worth double checking.

[Lestropie]

Yep, mentioned in OP. Was trivial to find them in this process, because following a mass class rename std::string_view::c_str() doesn't exist. The potential vulnerability is in future code feeding std::string_view::data() to a function that expects a null-terminated array. I'm not sure if there's any way to mitigate that other than code review & address sanitiser. But developers should no longer need to invoke anything that interfaces with a C-style array; any such C library functions we should have a wrapper around by this point.

[daljit46]

I'm not sure if there's any way to mitigate that other than code review & address sanitiser.

bugprone-suspicious-stringview-data-usage may be helpful here. I think the problem may only appear when interacting with libraries like fftw, but I agree that since this should be exceedingly rare, the current check is fine.

[github-actions]

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 25 out of 44. Check the log or trigger a new build to see more.

[github-actions]

warning: constructor does not initialize these fields: upper, lower [cppcoreguidelines-pro-type-member-init]

cpp/core/filter/zclean.h:273:

-   float upper, lower;
+   float upper{}, lower{};

[github-actions]

warning: constructor does not initialize these fields: zupper, zlower, fov_max, fov_min, bridge, dont_maskupper, keep_lower, keep_upper, upper, lower [cppcoreguidelines-pro-type-member-init]

cpp/core/filter/zclean.h:269:

-   float zupper, zlower;
-   float fov_max, fov_min;
-   size_t bridge;
-   bool dont_maskupper, keep_lower, keep_upper;
-   float upper, lower;
+   float zupper{}, zlower{};
+   float fov_max{}, fov_min{};
+   size_t bridge{};
+   bool dont_maskupper{}, keep_lower{}, keep_upper{};
+   float upper{}, lower{};

[github-actions]

warning: return type 'const SliceData' is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type]

Suggested change

	inline const SliceData parse_line(std::string_view line, const ParCols &cols) {
	inline SliceData parse_line(std::string_view line, const ParCols &cols) {

[github-actions]

warning: parameter 'equal_sizes' is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls]

Suggested change

	index_array_type load_blocks(std::string_view filename, const bool equal_sizes);
	index_array_type load_blocks(std::string_view filename, bool equal_sizes);

[github-actions]

warning: the const qualified parameter 's' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]

Suggested change

	auto erase = [&](const std::string s) {
	auto erase = [&](const std::string& s) {

[github-actions]

warning: variable 'previous_print_func' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]

  static void (*previous_print_func)(std::string_view msg);
                ^

[github-actions]

warning: variable 'previous_report_to_user_func' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]

  static void (*previous_report_to_user_func)(std::string_view msg, int type);
                ^

[github-actions]

warning: variable 'previous_report_to_user_func' provides global access to a non-const object; consider making the pointed-to data 'const' [cppcoreguidelines-avoid-non-const-global-variables]

  static void (*previous_report_to_user_func)(std::string_view msg, int type);
                ^

[github-actions]

warning: constructor accepting a forwarding reference can hide the move constructor [bugprone-forwarding-reference-overload]

  template <class Functor> __single_thread(Functor &&functor, std::string_view name = "unnamed") : __thread_base(name) {
                           ^

Additional context

cpp/core/thread.h:112: move constructor declared here

  __single_thread(__single_thread &&) = default;
  ^

[github-actions]

warning: redefinition of 'is_string_type' [clang-diagnostic-error]

template <class ValueType> struct is_string_type : std::false_type {};
                                  ^

Additional context

cpp/core/types.h:193: previous definition is here

template <class ValueType> struct is_string_type : std::false_type {};
                                  ^