Skip to content
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.

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

Remove strprintf printing for {fmt}. #2168

Closed
wants to merge 19 commits into from

Conversation

thorstenhater
Copy link
Contributor

Use libfmt in more place.

arbor/CMakeLists.txt Outdated Show resolved Hide resolved
arbor/fvm_layout.cpp Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the remainder still used anywhere? It is difficult to kill that usage too?

Copy link
Contributor

Choose a reason for hiding this comment

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

Tests I see, for to_string. What's wrong with std::to_string()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Glad you asked :D. util::to_string uses ADL (=argument dependent lookup) to select
to_string if present in namespace global or arb. If not it'll try to use << via std::stringstream.
std::to_string does not. This is why many classes in Arbor produce a friend << but do not
specialise to_string.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this is all a bit implicit. We should document that, for instance, all Arbor objects implement a public operator<< (as per #952). Maybe operator<< isn't really what we want, I suppose often you want to have the string output, so we could say that all Arbor objects also implement as_string() and that it has the same output as operator<<. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just learned that bit, so yes, a bit hidden. We can add something like this to our devdocs:

Arbor classes should implement << for debugging and user interface purposes.

Should be a should since it might never be needed and can be retrofitted easily.
I'd separate both concerns from this PR though, which is purely swapping out the substrate.

Regarding stream ./. strings:

  • Strings do allocate memory (repeatedly!), thus recursively stringifiing classes leads to potentially bad blowup, while streaming data out just needs a subset of the memory.
  • The memory concern will be especially bad for arrays/containers.
  • Streams have the issue of indirection via stringstream which util::to_string packages for you.

So, despite the slight convenience, I prefer the << way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding that line to docs would be good.

I meant a potential as_string() in addition to operator<< for the reasons you list.

util::to_string isn't great, because it's not quite the same as std::to_string (meant for numeric values only), so a bit confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, if the output should (MUST!) be the same, we end up with as_string(t) = (stringstream{} << t).str()
which is what util::to_string does anyhow. Also util::to_string falls back to std::to_string where
available (hence the ADL dance). See here

https://gcc.godbolt.org/z/r8KMEzYcj

  • Alice provides to_string and <<
  • Bob has just <<
  • double has std::to_string.

Thus, everybody will just be repeating util::to_string anyhow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sometimes inheritance from a base class is nice.

/ducks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeeees .... but (std::)to_string is standalone function and not a member or class.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeeees ... but .print() could just be an inherited method containing the string construction from << once and for all, without the separate-but-not-really function in util.

brenthuisman
brenthuisman previously approved these changes Aug 7, 2023
Copy link
Contributor

@brenthuisman brenthuisman left a comment

Choose a reason for hiding this comment

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

OK

@thorstenhater
Copy link
Contributor Author

Nice that you like this, but this is really not nearly done.

@brenthuisman
Copy link
Contributor

Then mark it draft, I thought we had agreed to do that?

@thorstenhater
Copy link
Contributor Author

Closing as it diverged too much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants