[ntuple] Fix [u]int64_t renormalization on Windows

[hahnjo]

... by handling the special __int64 type.

[github-actions]

Test Results

    20 files      20 suites   3d 8h 24m 37s ⏱️
 3 257 tests  3 257 ✅ 0 💤 0 ❌
63 502 runs  63 502 ✅ 0 💤 0 ❌

Results for commit f1a7326.

♻️ This comment has been updated with latest results.

[pcanal]

Can we extent the code comment? With the proposed code, it is unclear whether this is to handle the parameter input (in which case another 'or' on line line 270 and 271 would also make sense) or whether this is to fix up the value of ROOT::RField<long long int>::TypeName() (but then the question is why have that value be 'wrong').

[hahnjo]

The comment just above states that these are hit "during renormalization of demangled std::type_info names." That's very clearly not to fix up the value of ROOT::RField<long long int>::TypeName(). I don't know how to make this any clearer. The code does not belong into the other ifs (like lines 270 and 271) because there we don't know the bit-width of long, while again __int64 is very clear in that regard.

[pcanal]

The code does not belong into the other ifs (like lines 270 and 271) because there we don't know the bit-width
of long, while again __int64 is very clear in that regard.

Humm ... line 270 and 271 are about long long but still you are right we only know that there are 'at least' 64 bits (side note: I have not yet seen them be anything be 64 bits but indeed things could change).

That's very clearly not to fix up the value of ROOT::RField::TypeName().

if it is not to fix up the value of TypeName() then why (or more exactly why couldn't we have) the code be:

  } else if (typeName == "unsigned long long" || typeName == "unsigned long long int") {
      typeName = ROOT::RField<unsigned long long int>::TypeName();
   } else if (typeName == "__int64") {
      typeName = "std::int64_t";
   } else if (typeName == "unsigned __int64") {
      typeName = "std::uint64_t";
   }

The code in the PR read as:

   set typeName to RFied<>::TypeName based on input being standard C++ name spelling
   then
   override that value in case the input **or** the value returned by RFied<>::TypeName if seeing the Windows spelling

In practice there won't be a difference except for casting doubt in the future reader's mind on the values of RFied<>::TypeName

[hahnjo]

Yes, we could "merge" the added code into the existing if-else-cascade. I decided not to do that because a) it's a different kind of check (standard integer types vs fixed-width types specific to Windows) and b) it leaves a natural place to put the comment.

I can move the code if you insist (it won't make a difference), but I really wonder if it's worth spending our time on this kind of code layout questions...

[pcanal]

Suggested change

	}
	// The following two types are 64-bit integers on Windows that we can encounter during renormalization of demangled
	// std::type_info names.
	if (typeName == "__int64") {
	typeName = "std::int64_t";
	} else if (typeName == "unsigned __int64") {
	typeName = "std::uint64_t";
	}
	} else {
	// The following two types are 64-bit integers on Windows that we can encounter during renormalization of demangled
	// std::type_info names.
	if (typeName == "__int64") {
	typeName = "std::int64_t";
	} else if (typeName == "unsigned __int64") {
	typeName = "std::uint64_t";
	}
	}

but I really wonder if it's worth spending our time on this kind of code layout questions...

It is more than layout. It is control flow. My observation is that I, even after reading the comment, was left pondering whether the code was in practice hidden a (additional) issue in TypeName while the simple suggestion above achieve both goals of having a natural place for the comment and avoid an side-tracking of the future reader of the code.

[hahnjo]

I think this code layout is actively worse because it a) requires an additional level of indentation while it's generally considered better to try and reduce it, and b) because it's non-extensible: If we have another special case, we cannot add it the same way or require yet another level of indentation.

That said, I made the change to be finally able to merge this trivial fix after almost a week of (in my opinion) pointless discussions...