Skip to content

[DO NOT MERGE][ntuple] Use std::string_view consistently in signatures #22835

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

Draft
silverweed wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

[DO NOT MERGE][ntuple] Use std::string_view consistently in signatures #22835

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5e729bf
[ntuple] Replace all `const string &` with string_view in fn parameters
silverweed Jul 16, 2026
ab2d210
[io] Remove TFileMerger::SetMergeOptions overload taking const string&
silverweed Jul 16, 2026
53c2cc1
[core] Replace const char* with string_view in TClass::GetClass()
silverweed Jul 16, 2026
4688681
[core] Replace const string & with string_view in TClassEdit::CleanType
silverweed Jul 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/foundation/inc/TClassEdit.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ namespace TClassEdit {

void Init(TClassEdit::TInterpreterLookupHelper *helper);

std::string CleanType (const char *typeDesc,int mode = 0,const char **tail = nullptr);
std::string CleanType (std::string_view typeDesc,int mode = 0,const char **tail = nullptr);
inline bool IsArtificial(std::string_view name) { return name.find('@') != name.npos; }
bool IsDefAlloc(const char *alloc, const char *classname);
bool IsDefAlloc(const char *alloc, const char *keyclassname, const char *valueclassname);
Expand Down
11 changes: 6 additions & 5 deletions core/foundation/src/TClassEdit.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1287,7 +1287,7 @@ int TClassEdit::GetSplit(const char *type, vector<string>& output, int &nestedLo
/// CleanType(" A<B, C< D, E> > *,F,G>") returns "A<B,C<D,E> >*"
////////////////////////////////////////////////////////////////////////////

string TClassEdit::CleanType(const char *typeDesc, int mode, const char **tail)
string TClassEdit::CleanType(std::string_view typeDesc, int mode, const char **tail)
{
constexpr static std::array<const char *, 3> remove{"class", "const", "volatile"};
constexpr static auto lengths = []() constexpr {
Expand All @@ -1298,12 +1298,13 @@ string TClassEdit::CleanType(const char *typeDesc, int mode, const char **tail)
}();

string result;
result.reserve(strlen(typeDesc)*2);
result.reserve(typeDesc.size()*2);
int kbl=1;
std::vector<char> parensStack;
const char* c;
const char* c = nullptr;

for(c=typeDesc;*c;c++) {
for(auto i = 0u; i < typeDesc.size(); ++i) {
c = typeDesc.data() + i;
if (std::isspace(c[0])) {
if (kbl) continue;
if (!isalnum(c[ 1]) && c[ 1] !='_') continue;
Expand All @@ -1329,7 +1330,7 @@ string TClassEdit::CleanType(const char *typeDesc, int mode, const char **tail)
// make sure that the 'keyword' is not part of a longer indentifier
if (isalnum(c[rlen]) || c[rlen]=='_' || c[rlen]=='$') continue;

c+=rlen-1; done = 1; break;
c+=rlen-1; i+=rlen-1; done = 1; break;
}
if (done) continue;
}
Expand Down
4 changes: 2 additions & 2 deletions core/meta/inc/TClass.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -598,8 +598,8 @@ friend class TStreamerInfo;
static void AddClassToDeclIdMap(TDictionary::DeclId_t id, TClass* cl);
static void RemoveClass(TClass *cl);
static void RemoveClassDeclId(TDictionary::DeclId_t id);
static TClass *GetClass(const char *name, Bool_t load = kTRUE, Bool_t silent = kFALSE);
static TClass *GetClass(const char *name, Bool_t load, Bool_t silent, size_t hint_pair_offset, size_t hint_pair_size);
static TClass *GetClass(std::string_view name, Bool_t load = kTRUE, Bool_t silent = kFALSE);
static TClass *GetClass(std::string_view name, Bool_t load, Bool_t silent, size_t hint_pair_offset, size_t hint_pair_size);
static TClass *GetClass(const std::type_info &typeinfo, Bool_t load = kTRUE, Bool_t silent = kFALSE, size_t hint_pair_offset = 0, size_t hint_pair_size = 0);
static TClass *GetClass(ClassInfo_t *info, Bool_t load = kTRUE, Bool_t silent = kFALSE);
template<typename T>
Expand Down
34 changes: 19 additions & 15 deletions core/meta/src/TClass.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ In order to access the name of a class within the ROOT type system, the method T
#include "TThreadSlots.h"
#include "ThreadLocalStorage.h"

#include <ROOT/StringUtils.hxx>

#include <cstdio>
#include <cctype>
#include <set>
Expand Down Expand Up @@ -2996,26 +2998,28 @@ TVirtualIsAProxy* TClass::GetIsAProxy() const
/// (to anything) or set the `rootrc` key `Root.TClass.GetClass.AutoParsing` to
/// `false`.

TClass *TClass::GetClass(const char *name, Bool_t load, Bool_t silent)
TClass *TClass::GetClass(std::string_view name, Bool_t load, Bool_t silent)
{
return TClass::GetClass(name, load, silent, 0, 0);
}

TClass *TClass::GetClass(const char *name, Bool_t load, Bool_t silent, size_t hint_pair_offset, size_t hint_pair_size)
TClass *TClass::GetClass(std::string_view name, Bool_t load, Bool_t silent, size_t hint_pair_offset, size_t hint_pair_size)
{
if (!name || !name[0]) return nullptr;
if (name.empty()) return nullptr;

if (strstr(name, "(anonymous)")) return nullptr;
if (strstr(name, "(unnamed)")) return nullptr;
if (strncmp(name,"class ",6)==0) name += 6;
if (strncmp(name,"struct ",7)==0) name += 7;
if (name.find("(anonymous)") != std::string_view::npos) return nullptr;
if (name.find("(unnamed)") != std::string_view::npos) return nullptr;
if (ROOT::StartsWith(name, "class ")) name = name.substr(6);
if (ROOT::StartsWith(name, "struct ")) name = name.substr(7);

if (!gROOT->GetListOfClasses()) return nullptr;

std::string nameStr = std::string(name);

@pcanal pcanal Oct 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This (of course) means that at least here the std::string_view version is never faster than before (and often slower) :(

@silverweed silverweed Oct 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

"Often" is a bit of a guess, but yes, it would only become "faster" (assuming this is even relevant inside this complex function) if we trickled down the string_view signature to methods like TCollection::FindObject.

@pcanal pcanal Oct 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"Often" is a bit of a guess

Well the only cases where it is not slower are the case where the user is currently have to transform a std::string or std::string_view into a C string (and they would not pay the price a double allocation only if they change their code).

Now, I am not completely opposed to the change but I recommend we do not advertise is as the performance improvement (yet) [For this purpose the PR title is good but the PR description could be improved].

only become "faster" ... if we trickled down the string_view signature to methods like TCollection::FindObject.

True and more exactly if we do so for the majority of the code paths all the way down to the actual use.

@silverweed silverweed Oct 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Perhaps the wording on the PR comment is misleading, but this was never intended as a performance optimization. The "optimality" refers to programming convenience and doesn't want to make speed claims. It could become a performance optimization in the future but it would require consistently using string_view in a lot more places in the core/meta/IO code.

@pcanal pcanal Oct 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is fair but the text is explicitly calling out run-time performance:

Rationale: consistent use of string_view is more optimal than const char*/const string& due to less useless copies / string size checks / type conversions.
``
(see "optimal" and "less useless copies") - both are true only when (as you pointed out) string_view is used throughout the call stack.

Consider:

Rationale: using string_view consistently is more convenient and avoids making the user perform explicit conversions.


// FindObject will take the read lock before actually getting the
// TClass pointer so we will need not get a partially initialized
// object.
TClass *cl = (TClass*)gROOT->GetListOfClasses()->FindObject(name);
TClass *cl = (TClass*)gROOT->GetListOfClasses()->FindObject(nameStr.c_str());

// Early return to release the lock without having to execute the
// long-ish normalization.
Expand All @@ -3027,7 +3031,7 @@ TClass *TClass::GetClass(const char *name, Bool_t load, Bool_t silent, size_t hi
// Now that we got the write lock, another thread may have constructed the
// TClass while we were waiting, so we need to do the checks again.

cl = (TClass*)gROOT->GetListOfClasses()->FindObject(name);
cl = (TClass*)gROOT->GetListOfClasses()->FindObject(nameStr.c_str());
if (cl) {
if (cl->IsLoaded() || cl->TestBit(kUnloading))
return cl;
Expand Down Expand Up @@ -3063,7 +3067,7 @@ TClass *TClass::GetClass(const char *name, Bool_t load, Bool_t silent, size_t hi

// To avoid spurious auto parsing, let's check if the name as-is is
// known in the TClassTable.
if (DictFuncPtr_t dict = TClassTable::GetDictNorm(name)) {
if (DictFuncPtr_t dict = TClassTable::GetDictNorm(nameStr.c_str())) {
// The name is normalized, so the result of the first search is
// authoritative.
if (!cl && !load)
Expand Down Expand Up @@ -3099,7 +3103,7 @@ TClass *TClass::GetClass(const char *name, Bool_t load, Bool_t silent, size_t hi
// First look at known types but without triggering any loads
{
THashTable *typeTable = dynamic_cast<THashTable *>(gROOT->GetListOfTypes());
TDataType *type = (TDataType *)typeTable->THashTable::FindObject(name);
TDataType *type = (TDataType *)typeTable->THashTable::FindObject(nameStr.c_str());
if (type) {
if (type->GetType() > 0)
// This is a numerical type
Expand Down Expand Up @@ -3287,13 +3291,13 @@ TClass *TClass::GetClass(const char *name, Bool_t load, Bool_t silent, size_t hi
gInterpreter->GetInterpreterTypeName(normalizedName.c_str(), alternative, kTRUE);
if (alternative.empty())
return nullptr;
const char *altname = alternative.c_str();
if (strncmp(altname, "std::", 5) == 0) {
std::string_view altname = alternative;
if (ROOT::StartsWith(altname, "std::")) {
// For namespace (for example std::__1), GetInterpreterTypeName does
// not strip std::, so we must do it explicitly here.
altname += 5;
altname = altname.substr(5);
}
if (altname != normalizedName && strcmp(altname, name) != 0) {
if (altname != normalizedName && altname != name) {
// altname now contains the full name of the class including a possible
// namespace if there has been a using namespace statement.

Expand Down
1 change: 0 additions & 1 deletion io/io/inc/TFileMerger.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,6 @@ class TFileMerger : public TObject {
const char *GetMsgPrefix() const { return fMsgPrefix; }
void SetMsgPrefix(const char *prefix);
const char *GetMergeOptions() { return fMergeOptions; }
void SetMergeOptions(const TString &options) { fMergeOptions = options; }
void SetMergeOptions(const std::string_view &options) { fMergeOptions = options; }
void SetIOFeatures(ROOT::TIOFeatures &features) { fIOFeatures = &features; }
/// Add object names for PartialMerge().
Expand Down
4 changes: 2 additions & 2 deletions tree/ntuple/inc/ROOT/RField.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ protected:
ROOT::DescriptorId_t LookupMember(const ROOT::RNTupleDescriptor &desc, std::string_view memberName,
ROOT::DescriptorId_t classFieldId) const;
/// Sets fStagingClass according to the given name and version
void SetStagingClass(const std::string &className, unsigned int classVersion);
void SetStagingClass(std::string_view className, unsigned int classVersion);
/// If there are rules with inputs (source members), create the staging area according to the TClass instance
/// that corresponds to the on-disk field.
void PrepareStagingArea(const std::vector<const TSchemaRule *> &rules, const ROOT::RNTupleDescriptor &desc,
Expand Down Expand Up @@ -578,7 +578,7 @@ namespace Internal {
/// type renormalization of the demangled type name T. The failure case, however, needs to additionally check for
/// ROOT-specific special cases.
template <class T>
bool IsMatchingFieldType(const std::string &actualTypeName)
bool IsMatchingFieldType(std::string_view actualTypeName)
{
return IsMatchingFieldType(actualTypeName, ROOT::RField<T>::TypeName(), typeid(T));
}
Expand Down
11 changes: 5 additions & 6 deletions tree/ntuple/inc/ROOT/RFieldBase.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ void CallCommitClusterOnField(RFieldBase &);
void CallConnectPageSinkOnField(RFieldBase &, ROOT::Internal::RPageSink &, ROOT::NTupleSize_t firstEntry = 0);
void CallConnectPageSourceOnField(RFieldBase &, ROOT::Internal::RPageSource &);
ROOT::RResult<std::unique_ptr<ROOT::RFieldBase>>
CallFieldBaseCreate(const std::string &fieldName, const std::string &typeName, const ROOT::RCreateFieldOptions &options,
CallFieldBaseCreate(std::string_view fieldName, std::string_view typeName, const ROOT::RCreateFieldOptions &options,
const ROOT::RNTupleDescriptor *desc, ROOT::DescriptorId_t fieldId);

} // namespace Internal
Expand Down Expand Up @@ -97,7 +97,7 @@ class RFieldBase {
friend void Internal::CallConnectPageSinkOnField(RFieldBase &, ROOT::Internal::RPageSink &, ROOT::NTupleSize_t);
friend void Internal::CallConnectPageSourceOnField(RFieldBase &, ROOT::Internal::RPageSource &);
friend ROOT::RResult<std::unique_ptr<ROOT::RFieldBase>>
Internal::CallFieldBaseCreate(const std::string &fieldName, const std::string &typeName,
Internal::CallFieldBaseCreate(std::string_view fieldName, std::string_view typeName,
const ROOT::RCreateFieldOptions &options, const ROOT::RNTupleDescriptor *desc,
ROOT::DescriptorId_t fieldId);

Expand Down Expand Up @@ -566,7 +566,7 @@ protected:
/// normalized type name and type alias.
/// `desc` and `fieldId` must be passed if `options.fEmulateUnknownTypes` is true, otherwise they can be left blank.
static RResult<std::unique_ptr<RFieldBase>>
Create(const std::string &fieldName, const std::string &typeName, const ROOT::RCreateFieldOptions &options,
Create(std::string_view fieldName, std::string_view typeName, const ROOT::RCreateFieldOptions &options,
const ROOT::RNTupleDescriptor *desc, ROOT::DescriptorId_t fieldId);

public:
Expand Down Expand Up @@ -605,12 +605,11 @@ public:
/// Factory method to create a field from a certain type given as string.
/// Note that the provided type name must be a valid C++ type name. Template arguments of templated types
/// must be type names or integers (e.g., no expressions).
static RResult<std::unique_ptr<RFieldBase>>
Create(const std::string &fieldName, const std::string &typeName);
static RResult<std::unique_ptr<RFieldBase>> Create(std::string_view fieldName, std::string_view typeName);

/// Checks if the given type is supported by RNTuple. In case of success, the result vector is empty.
/// Otherwise there is an error record for each failing subfield (subtype).
static std::vector<RCheckResult> Check(const std::string &fieldName, const std::string &typeName);
static std::vector<RCheckResult> Check(std::string_view fieldName, std::string_view typeName);

/// Generates an object of the field type and allocates new initialized memory according to the type.
/// Implemented at the end of this header because the implementation is using RField<T>::TypeName()
Expand Down
14 changes: 7 additions & 7 deletions tree/ntuple/inc/ROOT/RFieldUtils.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,10 @@ namespace Internal {
/// template arguments (hence "Prefix").
/// Furthermore, if the type is a C-style array, rules are applied to the base type and the C style array
/// is then mapped to an std::array.
std::string GetCanonicalTypePrefix(const std::string &typeName);
std::string GetCanonicalTypePrefix(std::string_view typeName);

/// Given a type name normalized by ROOT meta, renormalize it for RNTuple. E.g., insert std::prefix.
std::string GetRenormalizedTypeName(const std::string &metaNormalizedName);
std::string GetRenormalizedTypeName(std::string_view metaNormalizedName);

/// Given a type info ask ROOT meta to demangle it, then renormalize the resulting type name for RNTuple. Useful to
/// ensure that e.g. fundamental types are normalized to the type used by RNTuple (e.g. int -> std::int32_t).
Expand All @@ -42,18 +42,18 @@ std::string GetRenormalizedTypeName(const std::type_info &ti);
/// to ensure correct reconstruction of objects from disk.
/// If the function returns true, renormalizedAlias contains the RNTuple normalized name that should be used as
/// type alias.
bool NeedsMetaNameAsAlias(const std::string &metaNormalizedName, std::string &renormalizedAlias,
bool NeedsMetaNameAsAlias(std::string_view metaNormalizedName, std::string &renormalizedAlias,
bool isArgInTemplatedUserClass = false /* used in recursion */);

/// Applies all RNTuple type normalization rules except typedef resolution.
std::string GetNormalizedUnresolvedTypeName(const std::string &origName);
std::string GetNormalizedUnresolvedTypeName(std::string_view origName);

/// Appends 'll' or 'ull' to the where necessary and strips the suffix if not needed.
std::string GetNormalizedInteger(const std::string &intTemplateArg);
std::string GetNormalizedInteger(std::string_view intTemplateArg);
std::string GetNormalizedInteger(long long val);
std::string GetNormalizedInteger(unsigned long long val);
long long ParseIntTypeToken(const std::string &intToken);
unsigned long long ParseUIntTypeToken(const std::string &uintToken);
long long ParseIntTypeToken(std::string_view intToken);
unsigned long long ParseUIntTypeToken(std::string_view uintToken);

/// Possible settings for the "rntuple.streamerMode" class attribute in the dictionary.
enum class ERNTupleSerializationMode {
Expand Down
2 changes: 1 addition & 1 deletion tree/ntuple/inc/ROOT/RFieldVisitor.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ The functions in this class format strings which are displayed by RNTupleReader:
class RNTupleFormatter {
public:
// Can abbreviate long strings, e.g. ("ExampleString" , space= 8) => "Examp..."
static std::string FitString(const std::string &str, int availableSpace);
static std::string FitString(std::string_view str, int availableSpace);
};

} // namespace Internal
Expand Down
4 changes: 2 additions & 2 deletions tree/ntuple/inc/ROOT/RMiniFile.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,8 +201,8 @@ private:
/// Writes a TKey including the data record, given by buffer, into fFile; returns the file offset to the payload.
/// The payload is already compressed
std::uint64_t WriteKey(const void *buffer, std::size_t nbytes, std::size_t len, std::int64_t offset = -1,
std::uint64_t directoryOffset = 100, const std::string &className = "",
const std::string &objectName = "", const std::string &title = "");
std::uint64_t directoryOffset = 100, std::string_view className = "",
std::string_view objectName = "", std::string_view title = "");
/// Reserves an RBlob opaque key as data record and returns the offset of the record. If keyBuffer is specified,
/// it must be written *before* the returned offset. (Note that the array type is purely documentation, the
/// argument is actually just a pointer.)
Expand Down
12 changes: 6 additions & 6 deletions tree/ntuple/inc/ROOT/RNTupleDescriptor.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1626,22 +1626,22 @@ public:
fField.fProjectionSourceId = id;
return *this;
}
RFieldDescriptorBuilder &FieldName(const std::string &fieldName)
RFieldDescriptorBuilder &FieldName(std::string_view fieldName)
{
fField.fFieldName = fStringPool->Intern(fieldName);
return *this;
}
RFieldDescriptorBuilder &FieldDescription(const std::string &fieldDescription)
RFieldDescriptorBuilder &FieldDescription(std::string_view fieldDescription)
{
fField.fFieldDescription = fStringPool->Intern(fieldDescription);
return *this;
}
RFieldDescriptorBuilder &TypeName(const std::string &typeName)
RFieldDescriptorBuilder &TypeName(std::string_view typeName)
{
fField.fTypeName = fStringPool->Intern(typeName);
return *this;
}
RFieldDescriptorBuilder &TypeAlias(const std::string &typeAlias)
RFieldDescriptorBuilder &TypeAlias(std::string_view typeAlias)
{
fField.fTypeAlias = fStringPool->Intern(typeAlias);
return *this;
Expand Down Expand Up @@ -1812,12 +1812,12 @@ public:
fExtraTypeInfo.fTypeVersion = typeVersion;
return *this;
}
RExtraTypeInfoDescriptorBuilder &TypeName(const std::string &typeName)
RExtraTypeInfoDescriptorBuilder &TypeName(std::string_view typeName)
{
fExtraTypeInfo.fTypeName = typeName;
return *this;
}
RExtraTypeInfoDescriptorBuilder &Content(const std::string &content)
RExtraTypeInfoDescriptorBuilder &Content(std::string_view content)
{
fExtraTypeInfo.fContent = content;
return *this;
Expand Down
Loading
Loading