Make interfaces use string_views more consistently

[tmadlener]

BEGINRELEASENOTES

• Make many of the reader and writer interfaces accept std::string_view arguments instead of const std::string& for categories, etc. to be more overall more consistent and to avoid having to do string_view to string conversions on the user side (see also #945).

ENDRELEASENOTES

This is a largely claude generated rework of current interfaces to fix the points raised in #945. I did some cleanup, but I will have to do another round to see where it actually makes sense to accept a string_view and where we were more consistent with taking a std::string.

• Release notes
• Includes changes from Make CI less sensitive about underlying environment #948

[tmadlener]

I have gone over things again and now there are only two maps left that actually have to have transparent std::string and std::string_view lookups. This also brings down the number of intermediate strings that were initially necessary quite a bit. In many of the cases we can get away with using a string_view as map key directly, because we know that the underlying string will outlive the map (e.g. because it lives in another vector<string> that gets populated first and remains unchanged afterwards.

In some cases the map still acts as owner of the initial string. For those cases we keep either the map with the transparent lookup or we internally construct a std::string to do whatever needs to be done.

@wdconinc our framework integration takes these changes transparently, which is what I would expect, but I can't as trivially test EICrecon with it. If you have an easy / quick way of doing that, I would appreciate any feedback about breakages.

[wdconinc]

If you have an easy / quick way of doing that, I would appreciate any feedback about breakages.

I'll give it a shot. Busy day today but hope to have a chance in between meetings.

[tmadlener]

Thanks and no worries. I still have to figure out why this makes some of the CI fail in code that hasn't been (obviously) changed.

[aashirvad08]

Looks like failure is coming from:

std::string_view(subrange.begin(), subrange.end())

std::views::split doesn’t provide contiguous iterators, so this isn’t a valid string_view construction (GCC 11 catches it)
Maybe construct a std::string instead, or avoid split_view if string_view is required?

[tmadlener]

The problem is that there are explicit guards to make this work with gcc11 in the code. And that code is not touched by this PR at all (it's not even called by code that is touched by this PR). So the question is why is a CI run ending up in a code section from which it should be guarded in the first place now when it hasn't before and nothing should have changed.

[aashirvad08]

Ah got it..then maybe something in this PR is changing which code paths get instantiated even if not directly touched.

Could it be affecting overload resolution or template instantiation so that guarded code is now being selected in gcc11?

[andresailer]

In file included from input_line_7:380:

This comes via root cling, so clang is defined, so it pulls in this code?

[tmadlener]

Good catch. I think then I know why. This PR makes MiscHelpers.h go through dictionary generation, where as previously it was only used for the podio-dump-tool, completely separate from anything ROOT does.

Sigh, time to split that header then, I guess. Unless someone wants to figure out the correct pre-processor chain of defined(XYZ) to make it pass for older ROOT versions.

[aashirvad08]

Happy to take a shot at the preprocessor path if useful
fix here: split std::string_view with find/substr instead of constructing from views::split subranges — those iterators aren't contiguous which is the root cause
if keeping the ranges path for newer toolchains, tighten the guard to CLING/ROOTCLING rather than clang since cling defining clang is what caused the leak into dictionary generation.

Splitting MiscHelpers.h is good hygiene on top but secondary to fixing the construction itself I think so

[tmadlener]

Splitting things off into a different header does the trick it seems. From my side this is ready and I would merge soon(ish), unless someone still wants more time for testing.

[jmcarcell]

Suggested change

	const std::string_view getDatamodelDefinition(std::string_view name) const;
	const std::string_view getDatamodelDefinition(const std::string_view name) const;

Is passing with const the best practice? I like to have const wherever it can be.

[tmadlener]

I haven't really seen it with const in practice (but maybe I have just looked in the wrong places). The underlying const char* is immutable in any case, so something like

const std::string_view getDatamodelDefinition(std::string_view name) {
  name[0] = 'a';
}

doesn't even compile.

You could do

  name = "abcd";

but those changes will not be visible outside (i.e. string_view doesn't behave like a span<char>).

I don't have a strong opinion here, but the convention seems to be without const.

[tmadlener]

Merging this tomorrow, unless there are more comments.