[RF] Fix parameter ordering bug in RooFormulaArgStreamer

[mark-owen]

This Pull request fixes:

The parameter ordering bug in RooFormulaArgStreamer. I changed the code to remove the vector< pair<RooAbsArg*, size_t> > and just directly loop in reverse order over the parameters of the formula.

Checklist:

• tested changes locally
• updated the docs (if necessary)

This PR fixes #17291

[guitargeek]

Suggested change

	std::vector<std::pair<std::size_t, const RooAbsArg*>> paramsWithIndex;
	std::vector<const RooAbsArg*> params;

As you suggested it now, the vector is now sorted by the index (see one of the following lines):

std::sort(paramsWithIndex.begin(), paramsWithIndex.end());

However, in this case the information of the index is redundant. The sort does nothing at all. The original code was badly designed.

Could you please refactor the code such that this redundant index is not stored and there is no std::sort? That means that the loops will then look something like this:

      for (std::size_t i = 0; i < params.size(); ++i) {
          RooAbsArg const *par = params[idx];
	  expression.ReplaceAll(("x[" + std::to_string(idx) + "]").c_str(), par->GetName());
      }

[mark-owen]

I realised even the vector is not really needed, so everything is now simplified to run in one (reverse order) loop.

[guitargeek]

Hi @mark-owen, thank you so much for this contribution!

Please make sure that the code is formatted with our clang-format style:
https://github.com/root-project/root/blob/master/.clang-format

Also, your changes made it apparent that the index information in the vector is redundant. Can you maybe change the code such that storing the index and sorting by it is avoided? Thanks.

[github-actions]

Test Results

    18 files      18 suites   4d 18h 1m 48s ⏱️
 2 682 tests  2 679 ✅ 0 💤 3 ❌
46 554 runs  46 551 ✅ 0 💤 3 ❌

For more details on these failures, see this check.

Results for commit faf11b5.

♻️ This comment has been updated with latest results.

[mark-owen]

Thanks @guitargeek - I see the point, the code can be simplified a bit (actually not sure the vector is needed at all). I'll update this in the next days (and look at clang-format).

[guitargeek]

Thank you!

[guitargeek]

Thank you so much for the fix! The fixes that also simplify the code are my favorite kind of fixes 🙂