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

Fix format conflict with std::format #419

Merged
merged 5 commits into from
Jul 17, 2023
Merged

Fix format conflict with std::format #419

merged 5 commits into from
Jul 17, 2023

Conversation

mtfishman
Copy link
Member

Trying to address #418.

The unfortunate part is that this relies on not introducing format from tinyformat into the namespace, so it would break user code that depends on just being able to directly use format.

I couldn't reproduce the original issue locally, @Wentzell could you see if this fixes the issue you saw? If it does then we can try to think of a safer fix.

@Wentzell
Copy link

With these changes I can successfully compile ITensor using gcc-13.
Thank you for looking into this so quickly @mtfishman

@mtfishman
Copy link
Member Author

@emstoudenmire any opinion on how to deal with the namespace clash of tinyformat::format, now that there is a std::format?

@mtfishman
Copy link
Member Author

@Wentzell glad that helped, I guess at least you and others can use this branch with the newer compilers while we decide on a more permanent solution.

@emstoudenmire
Copy link
Contributor

Thanks Matt, I think this is a good fix to just to not introduce format into the namespace. I think it's now better to leave it up to users whether they want to do something like using std::format; or using tfm::format; or just use the qualified names. Even though technically this is a breaking change, I think it's a very minor one in the sense of I wonder how many users were using format as introduced by ITensor?

@mtfishman
Copy link
Member Author

format is being used in some examples so I imagine some people copied it from there. I'm ok making this change, as you say it's easy enough to tell people to add using tfm::format; at the top of their code.

I guess with the new website we've lost our "News" section where we used to announce new versions and breaking changes for C++ ITensor, would be nice to find a place to announce this on the website so we don't catch people off guard. We can also announce things like that on the Discourse forum. For Julia we just do it in the Julia docs and Github README.

@emstoudenmire
Copy link
Contributor

emstoudenmire commented Jun 23, 2023 via email

@mtfishman
Copy link
Member Author

@emstoudenmire would you be ready to make a post about this if I merge it soon? @Wentzell wants this merged so they can use the official ITensor version in their CI tests.

@mtfishman
Copy link
Member Author

@emstoudenmire should I merge this?

@emstoudenmire
Copy link
Contributor

I've added a News entry publicly about this now on https://itensor.org

Please have a look to let me know if it's accurate.

@mtfishman
Copy link
Member Author

Thanks, looks great and that will be very helpful to have going forward! To be on the conservative side, I'm going to avoid introducing the tfm shorthand into the namespace by default, so users will have to use tinyformat::format. Could you change that "News" item appropriately? We could mention that the tfm shorthand can be used by defining:

namespace tfm = tinyformat;

@mtfishman mtfishman merged commit 06c04b8 into v3 Jul 17, 2023
1 check passed
@mtfishman mtfishman deleted the format_conflict branch July 17, 2023 14:29
@mtfishman
Copy link
Member Author

@emstoudenmire I updated the News item already.

@emstoudenmire
Copy link
Contributor

Thanks for updating it!

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.

None yet

3 participants