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

Switch to std::format #10294

Open
edolstra opened this issue Mar 22, 2024 · 3 comments
Open

Switch to std::format #10294

edolstra opened this issue Mar 22, 2024 · 3 comments
Labels
good first issue Quick win for first-time contributors

Comments

@edolstra
Copy link
Member

Is your feature request related to a problem? Please describe.

Unlike boost::format, std::format is supposed to check format strings at compile time, so it should prevent problems like #10293 once and for all.

Describe the solution you'd like
A clear and concise description of what you want to happen.

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context
Add any other context or screenshots about the feature request here.

Priorities

Add 👍 to issues you find important.

@edolstra edolstra added the feature Feature request or proposal label Mar 22, 2024
@roberth roberth added the good first issue Quick win for first-time contributors label Mar 22, 2024
@thufschmitt thufschmitt removed the feature Feature request or proposal label Mar 25, 2024
@SkamDart
Copy link
Contributor

I looked into this a bit and std::format does not seem to be very well supported across compilers yet. See Godbolt and currently supported native x86_64-linux gcc/clang toolchains.

Would it be acceptable to add {fmt} as a dependency until std::format is stabilized in the toolchains nix supports?

nix use-format → cat format_test.cc
#include <cassert>
#include <format>

int main()
{
    std::string message = std::format("The answer is {}.", 42);
    assert(message == "The answer is 42.");
}
nix use-format → clang++ -o format_test -std=c++2a format_test.cc
format_test.cc:2:10: fatal error: 'format' file not found
#include <format>
         ^~~~~~~~
1 error generated.
nix use-format → clang++ --version
clang version 16.0.6
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /nix/store/xph7agmqsna5ia7h699acfmk9gp4iks3-clang-16.0.6/bin
nix use-format → g++ --version
gcc (GCC) 12.3.0
Copyright (C) 2022 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

nix use-format → g++ -o format_test -std=c++2a format_test.cc
format_test.cc:2:10: fatal error: format: No such file or directory
    2 | #include <format>
      |          ^~~~~~~~
compilation terminated.

@edolstra
Copy link
Member Author

Yes that might be an option. Or we wait for GCC 13 and Clang 14.

@rahulharpal1603
Copy link

rahulharpal1603 commented Jun 6, 2024

Yes that might be an option. Or we wait for GCC 13 and Clang 14.

Hi, I am new to open source and have looked into this issue for a while now. If I am not wrong, our target file is this: https://github.com/NixOS/nix/blob/master/src/libutil/fmt.hh

Since std::format is now supported in gcc compiler version 13.1.0, we can replace boost::format with std::format. But, this will require many changes to the code base, mainly due to differences in the syntax of boost::format and std::format.

In the fmt.hh file, the class HintFmt is defined as a wrapper class for boost::format in the fmt.hh file. There are around 450 calls to HintFmt() in the entire codebase. In each call to HintFmt(), the first argument is a std::string e.g :-

In the file https://github.com/NixOS/nix/blob/master/src/libexpr/parser-state.hh at line 68, the following code is present:

inline void ParserState::dupAttr(Symbol attr, const PosIdx pos, const PosIdx prevPos)
{
    throw ParseError({
        .msg = HintFmt("attribute '%1%' already defined at %2%", symbols[attr], positions[prevPos]),
        .pos = positions[pos]
    });
}

if we are using std::format instead of boost::format, we must change the string "attribute '%1%' already defined at %2%" to "attribute '{0}' already defined at {1}".

We can replace these strings now, consider this call to HintFmt():
link: https://github.com/NixOS/nix/blob/master/src/libexpr/primops/fetchClosure.cc at line 161

    if (!fromPath)
        throw Error({
            .msg = HintFmt("attribute '%s' is missing in call to 'fetchClosure'", "fromPath"),
            .pos = state.positions[pos]
        });

Like printf(), boost::format supports format specifiers like %s , but std::format does not. So, we have to change attribute '%s' is missing in call to 'fetchClosure'" to attribute '{}' is missing in call to 'fetchClosure'" for using std::format.

We must also rewrite the HintFmt constructors and the operator overloads to support std::format.

Is this a correct approach? Please look into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Quick win for first-time contributors
Projects
None yet
Development

No branches or pull requests

5 participants