2
\$\begingroup\$

Exactly what the title says, this is a C++ class that attempts to read a file into a null-terminated string as efficiently as possible, using POSIX APIs. Obviously this is not intended to be portable code (other than between POSIX-compliant operating systems and GCC or clang), and the Slurp class would have a more extensive API in "real" code.

But, is there any room for improvement in terms of correctness or efficiency?

#ifndef SLURP_H
#define SLURP_H

class Slurp {
    void *begin_{nullptr};
    void *end_;
public:
    Slurp() = delete;
    Slurp(Slurp&) = delete;
    Slurp &operator=(Slurp) = delete;

    [[gnu::nonnull]] Slurp(const char path[]);

    ~Slurp();

    [[gnu::const, gnu::returns_nonnull]] const char *begin() const noexcept;
    [[gnu::const, gnu::returns_nonnull]] const char *end() const noexcept;
};

#endif //SLURP_H

// Slurp.cc
#include <fcntl.h>
#include <sys/mman.h>
#include <sys/stat.h>
#include <unistd.h>

#include <cerrno>
#include <cstdlib>
#include <cstring>
#include <system_error>

using namespace std;

#define ERROR_IF(CONDITION) do {\
        if (__builtin_expect((CONDITION), false))\
            throw system_error(errno, system_category());\
    } while (false)

#define ERROR_IF_POSIX(POSIX_PREFIXED_FUNCTION_CALL) do {\
        const int error_number = posix_##POSIX_PREFIXED_FUNCTION_CALL;\
        if (__builtin_expect(error_number, 0))\
            throw system_error(error_number, system_category());\
    } while (false)

Slurp::Slurp(const char path[]) {
    class FileDescriptor {
        const int fd_;
    public:
        FileDescriptor(int fd) : fd_{fd} {ERROR_IF(fd_ == -1);}
        ~FileDescriptor() {close(fd_);}
        [[gnu::const]] operator int() const noexcept {return fd_;}
    };

    FileDescriptor fd{open(path, O_RDONLY)};

    ERROR_IF_POSIX(fadvise(fd, 0, 0, POSIX_FADV_SEQUENTIAL));

    struct stat file_statistics;

    ERROR_IF(fstat(fd, &file_statistics));

    constexpr char32_t NullChar = U'\0';

    const size_t fsize = static_cast<size_t>(file_statistics.st_size),
        blksize = static_cast<size_t>(file_statistics.st_blksize),
        bufsize = fsize + sizeof NullChar;

    ERROR_IF_POSIX(memalign(&begin_, blksize, bufsize));

    end_ = static_cast<char*>(begin_) + fsize;

    ERROR_IF_POSIX(madvise(begin_, bufsize, POSIX_MADV_SEQUENTIAL));

    ERROR_IF(read(fd, begin_, fsize) == -1);

    memcpy(end_, &NullChar, sizeof NullChar); // ensure null termination
}

Slurp::~Slurp() {free(begin_);}

const char *Slurp::begin() const noexcept {return static_cast<char*>(begin_);}
const char *Slurp::end() const noexcept {return static_cast<char*>(end_);}
\$\endgroup\$
2
  • \$\begingroup\$ Why do you not trust the efficiency of fstream? Have you come to the conclusion that you need your own code based on an actual performance profile? \$\endgroup\$
    – Reinderien
    May 5 at 15:20
  • \$\begingroup\$ I was inspired by this blog post, where the (unspecified by the author) "POSIX" method of reading a file into a string was the most performant. \$\endgroup\$
    – Ray Hamel
    May 5 at 15:25
4
+50
\$\begingroup\$

This is a good project, one that could be very useful in practice. I’ve had two or three C++ blogs over the years, and every time I spin up a new one, I include an updated version of a very old post I wrote about reading an entire file into a string. It’s not an easy problem to solve efficiently, for a number of complicated reasons. Eschewing the standard library and going directly to underlying POSIX API is not a bad idea; one of the sources of inefficiency is the fact that the standard library will buffer everything, meaning you end up with an unnecessary “middle man” between your code and the actual file data.

I am not an expert in the POSIX APIs, so take everything I say in that area with a grain of salt. However, the algorithm itself looks okay; I don’t see anything wrong with it. There are a few things I think are probably unnecessary and useless, but nothing that introduces potential dangers. I’ll get into details about the algorithm as we come to them in the code.

Normally I would start with a design review, but… well, frankly, there doesn’t really look like there’s much in the way of design here. Your focus appears to be on the reading algorithm, and very little else. Which… not a good thing. That’s actually the opposite of how you write good code. And it shows here. To word it another way, the actual file-reading code is probably fine… but to actually use it in practical code (the way it’s currently written) would be difficult, at best.

For example, the iterators return char const*, and there looks like a lot of effort to NUL-terminate the data. So… is it meant to be string data? If so, why isn’t there a c_str() function? Or a conversion to string_view? On the other hand, if it’s not meant to be a string, just raw byte data read from a file, why not use std::byte? Why isn’t there a data() function?

Better yet, why not just read the data directly into an existing container, like std::string (for text data) or std::vector<std::byte> (for binary data)?

But I get it; your focus was on the read algorithm, not making an actually useful interface. That’s fine for experimentation, but when you’re eventually putting this code to use in a real-world library, the place that really needs focus is the public interface… not the low-level reading code. If your public interface sucks, then even if the reading code is the most brilliant code ever, the whole library still sucks for the user. But if your public interface is really good, then even if the reading code sucks… well, you can fix that under the hood, and users of your public interface won’t notice (except for maybe seeing performance gains).

Also, you should really get into the habit of testing your code. And by that I don’t mean “write it, then write a main() function that tries it out and if it all compiles and appears to run and produce correct-looking output, that’s that”. I mean fully unit-testing the code.

So, let’s get into the actual code.

Code review

Slurp.hh

    void *begin_{nullptr};
    void *end_;

The convention in C++ is to put all the type information together. In other words:

  • void *end_;: This is C style.
  • void* end_;: This is C++ style. (Well, other than that we try to avoid void* in C++.)

There is an alternative design to consider. Rather than holding two pointers, it might make more sense to hold a pointer and a size. Why? It makes it much easier to maintain and check the class invariant. If you use a pair of pointers, you have to make sure that end_ comes after begin_. But there is no portable way to ensure that (comparing random pointers is UB). On the other hand, if you use a pointer and a size, there is no possible way that the “end” can be before the beginning (assuming the size is unsigned, which is standard practice, but even if it’s signed, it’s trivial to check that the size is greater than zero).

It’s not really a big deal, and two pointers does work just fine in practice. It’s just something I’ve seen experts have opinions on.

I also don’t get why these are void*. void* makes a lot less sense in C++ than it does in C, and in this case… I can’t see the point of it at all. If this is supposed to be character data—a string—then why aren’t these char*? That sure seems to be the intention, what with all the NUL terminating and casting in begin() and end(). If this is supposed to be binary data, then unsigned char* or, better, std::byte* make more sense. void* just doesn’t make sense, not least because you’re just casting it to char* before using it anyway.

Anywho, aside from all that, there doesn’t seem to be a reason why end_ couldn’t also have an initializer.

    Slurp() = delete;

This line is technically unnecessary, because the fact that you have the constructor Slurp(char const[]) means that you have already disabled the default constructor.

Still, it doesn’t hurt, and it does express intent, so I’d leave it.

    Slurp(Slurp&) = delete;
    Slurp &operator=(Slurp) = delete;

These will probably “work”, but if your intention is to disable copying, then they are malformed. They should be:

    Slurp(Slurp const&) = delete;
    auto operator=(Slurp const&) -> Slurp& = delete;
    // or:
    //  Slurp& operator=(Slurp const&) = delete;

Disabling copying makes sense, given that copying will probably be fairly expensive. If someone really wants to copy the data, they can always do something like:

auto const data = Slurp{"input.data"};

auto const data_copy = std::vector(data.begin(), data.end());

But you might consider making the type movable. That could be very handy. It wouldn’t even be hard to do; you just need to make the class aware of a “null” or moved-from state, perhaps like so:

class Slurp
{
private:
    void* begin_ = nullptr;
    void* end_ = nullptr;

public:
    [[gnu::nonnull]] explicit Slurp(char const* path)
    {
        // your existing constructor code...
    }

    Slurp(Slurp&& other) noexcept
    {
        std::ranges::swap(begin_, other.begin_);
        std::ranges::swap(end_, other.end_);
    }

    ~Slurp()
    {
        if (begin_) // note the if test
            std::free(begin_);
    }

    auto operator=(Slurp&& other) noexcept -> Slurp&
    {
        std::ranges::swap(begin_, other.begin_);
        std::ranges::swap(end_, other.end_);
        return *this;
    }

    Slurp(Slurp const&) = delete;
    auto operator=(Slurp const&) -> Slurp& = delete;
};

Being able to very cheaply moved the slurped data around could be very handy indeed.

    [[gnu::nonnull]] Slurp(const char path[]);

Writing functions that look like they take arrays like this is a really bad idea. (Except for main(), which is just special in a lot of ways.) I’ve lost count of the number of newbies who think that this actually takes an array argument, and then think they can do sizeof(path) / sizeof(path[0]) to get the number of elements. Yes, you and I and other experts know what’s really going on, but you shouldn’t be writing code that’s only readable by experts. Down that path lies madness.

On the other hand Slurp(const char* path) or Slurp(char const* path) are unlikely to confuse beginners, and, really, tell the truth of what’s going on much better.

As a general rule, all constructors that take a single argument should be marked explicit (unless you have a damn good reason not to). Yes, explicit should really be the default, but that ship sailed a long time ago.

In addition to this constructor, it would help usability if you could use strings or—even better—actual paths with this class. It would be trivial to do, too:

explicit Slurp(std::string const& path) :
    Slurp{path.c_str()}
{}

explicit Slurp(std::filesystem::path const& path) :
    Slurp{path.c_str()}
{}

Now this next bit is going to get really complicated. Brace yourself.

gnu::const on non-static member functions

    [[gnu::const, gnu::returns_nonnull]] const char *begin() const noexcept;

The issue here the gnu::const attribute. I am not a GCC expert, but from my understanding of gnu::const, it sure seems like using it for a member function is an absolutely terrible, and dangerous, idea.

Here’s why.

gnu::const means that a function’s output depends ONLY on the function’s arguments. And I mean ONLY. The function’s behaviour cannot be affected by anything else; it doesn’t matter what state the rest of the program happens to be in (well, I mean, the state has to be valid… if the abstract machine’s state is kerfuckt, then all bets are off in any case), the function’s return value will ALWAYS be the same, given the same arguments.

This makes sense for, say, a function like: auto twice(int i) { return i * 2; }. It doesn’t matter what’s going on with the rest of the program, like how many exceptions are in flight or how many files are open or how much memory has been allocated. So long as the function can complete without UB (for example, there is no stack overflow), then it will ALWAYS return the same result for the same arguments. twice(32) will ALWAYS return 64. ALWAYS. GCC can use that information to optimize. For example, if you call twice(14) a million times in your program, GCC might cache the result of the first call, and then replace every other call with just loading that cached value.

So far none of this sounds problematic, I suppose. Well, now let’s talk about non-static member functions.

There is a very important key feature of non-static class member functions: the implicit this.

You see, every non-static class member function has a hidden this argument. We usually pretend it’s the first argument (but it doesn’t actually have to be, in reality, it’s usually passed in a specific register), so your begin() function actually looks like begin(Slurp const*):

// what you write:
auto Slurp::begin() const noexcept -> char const*
{
    return begin_;
}

// what the compiler actually implements:
auto Slurp__begin(Slurp const* this) noexcept -> char const*
{
    return this->begin_;
}

Normally, this isn’t something you need to think about.

However… remember what gnu::const means. It means that begin() will ALWAYS return the same result, when called with the same argument.

So let’s imagine you write this:

auto const s = Slurp{"data.file"};

auto p1 = s.begin();
auto p2 = s.begin();
auto p3 = s.begin();
auto p4 = s.begin();
auto p5 = s.begin();

The above is theoretically:

auto const s = Slurp{"data.file"};

auto p1 = Slurp__begin(&s);
auto p2 = Slurp__begin(&s);
auto p3 = Slurp__begin(&s);
auto p4 = Slurp__begin(&s);
auto p5 = Slurp__begin(&s);

But since begin() is gnu::const, once we know the result of calling it with address of s, we can assume that will ALWAYS be the result of calling it with that address. So the compiler could do:

auto p1 = Slurp__begin(&s);
auto p2 = p1;
auto p3 = p1;
auto p4 = p1;
auto p5 = p1;

(And it probably will.)

So far no problems. There is currently no way for the value of begin_ to change once a Slurp object has been constructed, so the above code is a perfectly valid optimization.

However, suppose you implemented moving—or copying, or any way to change the data in a Slurp object. Then you wrote this:

auto const s = Slurp{"data.file"};

auto p1 = s.begin();
auto p2 = s.begin();

s = Slurp{"other.file"};    // this line will construct a new Slurp,
                            // and REPLACE the internals of `s` with the
                            // new data

auto p3 = s.begin();
auto p4 = s.begin();

Now the address of s hasn’t changed… but it’s internal data—in particular, the value of begin_has changed. The compiler sees the above code as:

auto const s = Slurp{"data.file"};

auto p1 = Slurp__begin(&s);
auto p2 = Slurp__begin(&s);

s = Slurp{"other.file"};

auto p3 = Slurp__begin(&s);
auto p4 = Slurp__begin(&s);

And it does the optimization:

auto const s = Slurp{"data.file"};

auto p1 = Slurp__begin(&s);
auto p2 = p1;

s = Slurp{"other.file"};

auto p3 = p1;
auto p4 = p1;

Except after the move assignment, p1 is now a dangling pointer. Which means p3 and p4 are dangling pointers, too.

This is a disaster.

Now you may be thinking: “Meh, no problem; my class is non-copyable, non-movable, and immutable. This problem doesn’t affect me.”

Wanna bet?

Remember, gnu::const isn’t about the object. It’s about the arguments to the function. In the example above, the object’s internals changed, but the address of the object didn’t, so the argument to begin() didn’t either, and that was the problem. So if the object’s internals can’t possibly change… no problem, right?

Wrong.

Because even if it is impossible to change the internals of an object, it is still possible to create an object… destroy it… then create a whole new object at the same memory address… and it is the address that matters. Because that is what is being used as the function argument to begin().

Here’s just one example of how that could happen:

auto s = std::optional<Slurp>{std::in_place, "data.file"};

auto p1 = s->begin();
auto p2 = s->begin();

s.emplace("other.file");    // destroys contained Slurp object, then
                            // constructs a new one in the same place... so it
                            // has the same address

auto p3 = s->begin();
auto p4 = s->begin();

Which eventually optimizes to:

auto s = std::optional<Slurp>{std::in_place, "data.file"};

auto p1 = s->begin();
auto p2 = p1;

s.emplace("other.file");

auto p3 = p1;
auto p4 = p1;

And ends up with the same problem.

But you don’t even need std::optional! This problem could be triggered with even this code:

auto f()
{
    auto s = Slurp{"data.file"};

    auto p = s.begin();
}

auto some_other_function()
{
    f();
    f();
}

What might happen above is the first call to f() happens to place s at some address on the stack… then when f() ends, s is destroyed… then f() is called again and just happens to place the new s at the same address on the stack. But of course, the allocation done within the Slurp constructor (the posix_memalign() call) may be entirely different. So p SHOULD have two entirely different values with the two successive calls to f() even though the address of s (and thus, the argument passed to begin()) is the same for both calls. But gnu::const says it won’t; gnu::const says that if the argument is the same, the result is the same.

Once again: catastrophe.

To get a clearer view of why, imagine the calls to f() are inlined. Inlining isn’t necessary to trigger the problem, but it helps illustrate. If the calls are inlined, some_other_function() will look like:

auto some_other_function()
{
    {
        auto s = Slurp{"data.file"};

        auto p = s.begin();

        // s is destroyed here
    }

    {
        auto s = Slurp{"data.file"};

        auto p = s.begin();

        // s is destroyed here
    }
}

After the first s is destroyed, the second s may very well be constructed at the exact same location… let’s call it STACK_ADDRESS. So:

auto some_other_function()
{
    {
        auto s = Slurp{"data.file"};

        auto p = Slurp__begin(STACK_ADDRESS);

        // s is destroyed here
    }

    {
        auto s = Slurp{"data.file"};

        auto p = Slurp__begin(STACK_ADDRESS);

        // s is destroyed here
    }
}

The two calls to begin() are identical, and gnu::const promises that that means the result will be identical, so the compiler might do:

auto some_other_function()
{
    auto __p_inlined = static_cast<char const*>(nullptr);

    {
        auto s = Slurp{"data.file"};

        __p_inlined = Slurp__begin(STACK_ADDRESS);
        auto p = __p_inlined;

        // s is destroyed here
    }

    {
        auto s = Slurp{"data.file"};

        auto p = __p_inlined;

        // s is destroyed here
    }
}

And, boom.

Whew, that was a long digression. The tl;dr is “don’t use gnu::const for (non-static) member functions”… but I think it’s important to understand why.

And, really, it seems kinda pointless in modern C++. If it is possible for a function to be gnu::const, then it is almost certainly also possible for it to be constexpr… in which case the compiler can optimize it far more aggressively than it could possibly optimize a function that is merely gnu::const (but not wholly visible).

Slurp.cc

using namespace std;

Never, ever do this at file scope. No, not even in a non-header file. Just use std::.

#define ERROR_IF(CONDITION) do {\
        if (__builtin_expect((CONDITION), false))\
            throw system_error(errno, system_category());\
    } while (false)

#define ERROR_IF_POSIX(POSIX_PREFIXED_FUNCTION_CALL) do {\
        const int error_number = posix_##POSIX_PREFIXED_FUNCTION_CALL;\
        if (__builtin_expect(error_number, 0))\
            throw system_error(error_number, system_category());\
    } while (false)

Avoid macros. There’s no good reason for them here. Inlined functions will be just as efficient… not that it really matters, because these are error cases after all. For example:

namespace {

auto error_if(int result)
{
    if (result != 0) [[unlikely]]
        throw std::system_error{errno, std::system_category()};
}

auto error_if_posix(int error_number)
{
    if (error_number != 0) [[unlikely]]
        throw std::system_error{error_number, std::system_category()};
}

} // anonymous namespace

Slurp::Slurp(char const* path)
{
    // ... [snip] ...

    error_if_posix(::posix_fadvise(fd, 0, 0, POSIX_FADV_SEQUENTIAL));

    struct ::stat file_statistics{};
    error_if(::fstat(fd, &file_statistics));

    // ... etc.

But even that’s not really great, because it just obfuscates the function’s logic. I don’t see how it’s really an improvement over:

Slurp::Slurp(char const* path)
{
    // ... [snip] ...

    if (auto const res = ::posix_fadvise(fd, 0, 0, POSIX_FADV_SEQUENTIAL); res != 0) [[unlikely]]
        throw std::system_error{res, std::system_category()};

    struct ::stat file_statistics{};
    if (auto const res = ::fstat(fd, &file_statistics); res != 0) [[unlikely]]
        throw std::system_error{errno, std::system_category()};

    // ... etc.

The only thing I might do is maybe avoid repeating std::system_category by doing:

namespace {

auto posix_error(int err)
{
    return std::system_error{err, std::system_category()};
}

} // anonymous namespace

Slurp::Slurp(char const* path)
{
    // ... [snip] ...

    if (auto const res = ::posix_fadvise(fd, 0, 0, POSIX_FADV_SEQUENTIAL); res != 0) [[unlikely]]
        throw posix_error(res);

    auto file_statistics = ::stat{};
    if (auto const res = ::fstat(fd, &file_statistics); res != 0) [[unlikely]]
        throw posix_error(errno);

    // ... etc.

I just don’t see the benefit of hiding the throws. Yes, I get that it is somewhat ugly to have to check the return code every function call… but this is a C API; that’s just the nature of the game.

If you really don’t want all that ugly C-ish code, the right move is to properly wrap the API… not to obfuscate your logic by hiding it behind macros. For example:

Slurp::Slurp(char const* path)
{
    auto const fd = posix::open(path, O_RDONLY);

    posix::fadvise(fd, 0, 0, POSIX_FADV_SEQUENTIAL);

    auto const file_statistics = posix::fstat(fd);

    // ... etc.

(Note that implied is that each call above throws on failure, like a good C++ API should.)

But it’s really an all-or-nothing deal. Either completely wrap the API into a proper C++ API, so the usage just looks like regular C++… which maintainers of the code should be familiar with. Or use the C API as-is—don’t hide it behind macros—so people who actually know the POSIX API inside and out can look at your code and immediately spot what’s going on (and any potential problems). Going half-way just annoys both parties, with no real benefits.

    class FileDescriptor {
        const int fd_;
    public:
        FileDescriptor(int fd) : fd_{fd} {ERROR_IF(fd_ == -1);}
        ~FileDescriptor() {close(fd_);}
        [[gnu::const]] operator int() const noexcept {return fd_;}
    };

This is a really useful class! Rather than burying it as an implementation detail, it could be part of a public API. You’d need to flesh it out a bit (and make it non-copyable, but perhaps movable), but it’s pretty cool.

FileDescriptor fd{open(path, O_RDONLY)};

This goes back to the “never use using namespace std;” rule: function names like open() are extremely common, and there is a very high chance of conflict between the POSIX function, and some function you might put in your class in the future. If that were to happen, if you are lucky, you’ll get a compile error… but I’ve seen cases where behaviour subtly changes because the wrong function is called.

To make sure you get the right function, you should explicitly qualify it. In this case, because it’s a POSIX function that doesn’t exist in a namespace, you have to use ::open().

You should do this for all the POSIX functions.

constexpr char32_t NullChar = U'\0';

I’m a little baffled by the logic here.

Okay, so you want the data to be null-terminated. Fine, sure. But… you’re treating the data as an array of char… not char32_t. So… huh?

The only thing I can figure is that you’re imagining that maybe someone might possibly want to reinterpret_cast the char const* to char32_t const*, and treat the data as a UTF-32 NUL-terminated string. That seems a pretty odd thing to presume. And it’s even odder when you consider that the Slurp class doesn’t even model a NUL-terminated string in any sense. There is no c_str() function, for example. Even if there were, it’s hard to see why it should return a char32_t const* and not a char const*.

Aside from being odd, it may also be wrong. You are aligning the memory according to file_statistics.st_blksize… not alignof(char32_t). In other words, if you’re thinking that you can just reinterpret_cast the result of begin() to a char32_t const* and treat it as a UTF-32 string… you may be in for a nasty surprise when your program crashes due to a a misaligned read. Or, possibly, just garbled text (that is probably illegal Unicode). (And all this is assuming the endianness of the UTF-32 data is correct.)

I mean, if you really wanted to read file data as a UTF-32 string, why wouldn’t you read it into a std::u32string? Similarly, if you want a regular string, why not read it directly into a std::string? In either case, all the allocation and alignment headaches are taken care of, and the NUL termination is automatic. And if you want binary data, why not read directly into a std::vector<std::byte>?

I’ll be getting back to that idea later.

ERROR_IF_POSIX(memalign(&begin_, blksize, bufsize));

Honestly, I don’t see the point of this.

Okay, I get that the file you’re reading probably has a preferred block size, and if you were mem-mapping the file, or reading it directly from storage, then yes, you’d need aligned memory. But you’re doing neither.

Internally, when you open the file (and specify the access characteristics via posix_fadvise()) a buffer gets created. (I think you can avoid this by using direct input, but that’s beyond the scope here. mmap() is another way to avoid the buffer, but that’s another beast altogether.) Then you are reading from that buffer into your program’s memory space. Now, the buffer needs to be aligned to the block size of the file, so that the file can be read efficiently a block at a time. But your memory… the memory you allocate to read the file data (from the buffer) into… that does not need to be aligned to the block size of the file… because with that memory, you are not reading directly from the file, you are reading memory-to-memory (from the kernel’s buffer, to your array).

Follow? Block-alignment only matters for the initial read of the file from the device. THAT needs to be block-aligned. But unless you’re doing direct input, that’s happening in the kernel (usually), and then read() copies from that buffer to your desired location. Once the data is already in RAM, it’s just a memory-to-memory copy… which is just a rep stosb or some SSE or AVX operation. That doesn’t need to be aligned.⁕

⁕ (Ugh, in order to make this not too absurdly long, I have to gloss over or outright ignore a lot of hyper-technical details, so I know there are nerds out there just champing at the bit to “akshually…” me in the comments. Okay, yes, the data does need to be aligned for super-fast memory-to-memory transfer… both the source and destination data, with penalties for not being aligned varying depending on your processor architecture. HOWEVER… the default C++ allocator in any serious-grade standard library is almost certainly already aligning every allocation to take advantage of those efficiencies by default. So unless you’re doing something “clever”, you don’t need to worry about it.)

In other words, you’re wasting your time using posix_memalign(). You gain nothing from it. You could just as well be using malloc() or new.

Which brings us back again to the idea of reading directly into std::string or std::vector<std::byte>. But let me postpone that for a bit more, because we’re almost at the end of the code, so let’s get through that first.

ERROR_IF_POSIX(madvise(begin_, bufsize, POSIX_MADV_SEQUENTIAL));

I’ll admit that I don’t understand how this call isn’t failing. I thought posix_madvise() was only for memory-mapped memory. I would have guessed you’d get a EBADF from trying to use it on random memory. TIL, I guess.

But I do suspect the call is just being ignored. My understanding of POSIX_MADV_SEQUENTIAL is that it signals to the kernel to make a larger look-ahead buffer… which makes sense if you’re mem-mapping a file into memory, but is just nonsense for RAM. (Where you gonna put the look-ahead buffer for RAM? Super-duper-RAM?)

ERROR_IF(read(fd, begin_, fsize) == -1);

read() can fail spuriously for a number of reasons, like a signal interrupting it, and end up doing only a partial read. You can’t just write a read() call like this; you need to call it in a loop and keep doing reads until you’ve read fsize bytes in total or until read() returns zero.

memcpy(end_, &NullChar, sizeof NullChar); // ensure null termination

I mean… why not just std::fill_n(end, 4, 0);? It would probably be faster. (Unless your optimizer is smart enough to transform that memcpy() into a memset(), in which case it will probably be equally fast.)

Reading directly into containers

Okay, the biggest problem with Slurp is that it’s a specialized type. Even if it had a perfect interface, it’s still not a string. If I need a std::string, which is very common, then I’ll be forced to copy the entire Slurp object into a std::string. What a waste.

What if you just read the file’s contents directly into a std::string. Or, if it’s binary data, into a std::vector<std::byte>. Remember, there is no need to use posix_memalign() to specially align the memory to the original file’s preferred block size. The default allocator will align things just fine. So you can just use standard containers.

(But, FYI, even if the default allocator didn’t align things just fine, it would still be a better solution to use a custom allocator, and standard containers, rather that rolling your own, half-assed container.)

For example, to read into a string (note: untested, rough code):

auto read_into_string(std::filesystem::path const& path)
{
    // file_descriptor is a class basically identical to yours
    auto const fd = file_descriptor{::open(path.c_str(), O_RDONLY)};

    // get the file size from the file descriptor
    auto const file_size = [&]
    {
        struct ::stat fstats{};
        if (auto const res = ::fstat(fd, &fstats); res != 0)
            throw std::system_error{errno, std::system_category()};
        return fstats.st_size;
    }();

    // create the string we'll be reading into
    auto s = std::string{};
    s.resize(file_size);

    // this is the read loop
    auto pos = std::string::size_type{};
    while (true)
    {
        auto const last_read = ::read(fd, s.data() + pos, s.size() - pos);
        if (last_read < 0)
            throw std::system_error{errno, std::system_category()};
        else if (last_read == 0)
            break;

        // this should never happen
        if (last_read > (s.size() - pos)) [[unlikely]]
            throw std::runtime_error{"more data in file than file size"};

        pos += last_read;
    }

    // this should never happen
    if (pos != s.size()) [[unlikely]]
        throw std::runtime_error{"failed to read entire file"};

    return s;
}

And the usage:

auto const string_data = read_into_string("file.txt");

And string_data is a std::string.

The only thing that I suspect might make this code slower than your code, and only for very, very large files, is that there is no way (AFAIK) to allocate a std::string without initializing its contents. In other words, s.resize(n) doesn’t does just allocate n bytes (internally), it also zeros those bytes… which is a total waste of time for us. You shouldn’t have this inefficiency if you’re reading binary data into a std::vector<std::byte> (for which the code is otherwise identical).

But even that std::string inefficiency should be tolerable (unless your files are huge), because you get the benefit of actually having an honest-to-goodness string. Which basically works everywhere, and which many APIs specifically need, in one form or another.

And, frankly, I seriously doubt you’ll even notice it, because, again, unless you’re dealing with huge files, the cost of that zeroing will be completely dwarfed by the I/O costs.

Anywho, this is getting way too long, and I’ve been sitting on it for days, so, I’ll just go ahead and publish what I’ve got.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for giving me such a thorough answer! My detailed response to you is in my own answer below. \$\endgroup\$
    – Ray Hamel
    May 13 at 1:05
0
\$\begingroup\$

I would expect Slurp.cc to include Slurp.h - in fact, I can't see how you could compile it without the definitions there. I recommend making it the first include, to detect whether any dependencies are introduced.

using namespace std;

Bringing all of the std identifiers into the global namespace is usually a bad idea - just write std::size_t and other identifiers in full.

It seems strange to use a std::char32_t as terminator for a string of char. If we need to read files with different character types, then I would expect to handle that using a template class.

Obviously files with embedded null characters will appear truncated when interpreted as (null-terminated) C strings; it's good that you provide begin() and end() so users can wrap in std::string_view - although start and length would feed better to its constructor.

\$\endgroup\$
1
  • \$\begingroup\$ 1) In case it wasn't clear, my code snippet was supposed to be Slurp.cc after Slurp.h was included. 2) It's just char32_t, no std:: (it's a fundamental type). 3) The character encoding (or lack thereof) of a file is not necessarily known at compile-time so I find it most correct to always handle the case where the file is encoded as UTF-32. In "real" code Slurp would provide different "views" or iterators for different character encodings. \$\endgroup\$
    – Ray Hamel
    May 12 at 20:58
0
\$\begingroup\$

Here is how I've updated my code, for C++20, to make the API more usable and full-featured, and with feedback from indi. After that I've written a response to indi's feedback, since I think he deserves one and it's too long and detailed to do so in comments.


Demangle.h

#ifndef DEMANGLE_H
#define DEMANGLE_H

#include <cxxabi.h>

#include <cstdlib>
#include <new>
#include <string_view>
#include <typeinfo>

template<typename T>
class Demangle {
    char *name{nullptr};

    Demangle(std::string_view s)
        : name{static_cast<char*>(std::calloc(s.size() + 1, 1))}
    {
        if (!name)
            throw std::bad_alloc{};

        s.copy(name, s.size());
    }
public:
    Demangle() {
        int status;
        name = abi::__cxa_demangle(
            typeid(T).name(), nullptr, nullptr, &status);

        if (!name)
            throw std::bad_alloc{};
    }

    Demangle(const Demangle &other) : Demangle{std::string_view{other}} {}

    Demangle(Demangle &&other) noexcept {std::swap(name, other.name);}

    Demangle &operator=(Demangle rhs) noexcept {
        std::swap(name, rhs.name);
        return *this;
    }

    ~Demangle() {std::free(name);}

    operator std::string_view() const noexcept {return name;}
};

#endif//DEMANGLE_H

FileDescriptor.h

#ifndef FILE_DESCRIPTOR_H
#define FILE_DESCRIPTOR_H

class FileDescriptor {
    const int fd_;
public:
    explicit FileDescriptor(int fd);
    [[gnu::nonnull]] FileDescriptor(const char path[], int oflag);
    /* std::string / std::filesystem::path / similar constructor
     */
    template<class T>
    FileDescriptor(const T &t, int oflag)
        : FileDescriptor{t.c_str(), oflag} {}

    ~FileDescriptor();

    [[gnu::const]] operator const int&() const noexcept;
};

#endif//FILE_DESCRIPTOR_H

ProjectConcepts.h

#ifndef PROJECT_CONCEPTS_H
#define PROJECT_CONCEPTS_H

#include <type_traits>

namespace Project {

template<typename T>
concept TriviallyCopyable = std::is_trivially_copyable_v<T>;

}//Project

#endif//PROJECT_CONCEPTS_H

Slurp.h

#ifndef SLURP_H
#define SLURP_H

#include "Demangle.h"
#include "FileDescriptor.h"
#include "ProjectConcepts.h"

#include <filesystem>
#include <initializer_list>
#include <span>
#include <stdexcept>
#include <string>

class Slurp {
    template<typename T>
    static constexpr bool IsSmol = sizeof(T) == 1;

    struct Bounds {void *begin{nullptr}, *end;} b;

    template<Project::TriviallyCopyable T>
    std::initializer_list<T*> init() const noexcept(IsSmol<T>) {
        if (size() % sizeof(T)) [[unlikely]] {
            const Demangle<T> tp_name;
            throw std::logic_error{std::string{
                "can't interpret file as array of "} + tp_name};
        }
        return {static_cast<T*>(b.begin), static_cast<T*>(b.end)};
    }
public:
    Slurp(const Slurp&) = delete;
    Slurp &operator=(const Slurp&) = delete;

    Slurp(Slurp &&other) noexcept;
    Slurp &operator=(Slurp &&rhs) noexcept;

    explicit Slurp(const FileDescriptor &fd);
    [[gnu::nonnull]] Slurp(const char path[]);
    explicit Slurp(const std::string &path);
    explicit Slurp(const std::filesystem::path &path);

    ~Slurp();

    [[gnu::returns_nonnull]] const char *begin() const noexcept;
    [[gnu::returns_nonnull]] const char *end() const noexcept;

    std::size_t size() const noexcept;

    template<Project::TriviallyCopyable T, class Traits=std::char_traits<T>>
    operator std::basic_string_view<T, Traits>() const noexcept(IsSmol<T>) {
        return init<T>();
    }
    template<Project::TriviallyCopyable T>
    operator std::span<T>() & noexcept(IsSmol<T>) {
        return init<T>();
    }
    template<Project::TriviallyCopyable T>
    operator std::span<const T>() const noexcept(IsSmol<T>) {
        return init<T>();
    }
};

#endif//SLURP_H

SystemErrorIf.h

#ifndef SYSTEM_ERROR_IF_H
#define SYSTEM_ERROR_IF_H

#include <cerrno>
#include <system_error>

namespace SystemErrorIf {

constexpr void condition(bool condition) {
    if (condition) [[unlikely]]
        throw std::system_error{errno, std::system_category()};
}

constexpr void code(int error_number) {
    if (error_number) [[unlikely]]
        throw std::system_error{error_number, std::system_category()};
}

}//SystemErrorIf

#endif//SYSTEM_ERROR_IF_H

FileDescriptor.cc

#include "FileDescriptor.h"
#include "SystemErrorIf.h"

#include <fcntl.h>
#include <unistd.h>

FileDescriptor::FileDescriptor(int fd) : fd_{fd} {
    SystemErrorIf::condition(fd_ == -1);
}
FileDescriptor::FileDescriptor(const char path[], int oflag)
    : FileDescriptor{::open(path, oflag)} {}

FileDescriptor::~FileDescriptor() {::close(fd_);}

FileDescriptor::operator const int&() const noexcept {return fd_;}

Slurp.cc

#include "Slurp.h"
#include "SystemErrorIf.h"

#include <fcntl.h>
#include <sys/mman.h>
#include <sys/stat.h>
#include <unistd.h>

#include <cstring>

Slurp::Slurp(Slurp &&other) noexcept {std::swap(b, other.b);}

Slurp &Slurp::operator=(Slurp &&rhs) noexcept {
    std::swap(b, rhs.b);
    return *this;
}

Slurp::Slurp(const FileDescriptor &fd) {
    namespace ErrorIf = SystemErrorIf;

    ErrorIf::code(::posix_fadvise(fd, 0, 0, POSIX_FADV_SEQUENTIAL));

    struct ::stat file_statistics;

    ErrorIf::condition(::fstat(fd, &file_statistics));

    ::ssize_t fsize = file_statistics.st_size;

    const auto bufsize = static_cast<std::size_t>(fsize) + sizeof(char32_t),
        blksize = static_cast<std::size_t>(file_statistics.st_blksize);

    ErrorIf::code(::posix_memalign(&b.begin, blksize, bufsize));

    ErrorIf::code(::posix_madvise(b.begin, bufsize, POSIX_MADV_SEQUENTIAL));

    b.end = b.begin;

    while (fsize > 0) {
        const auto nread = ::read(
            fd, b.end, static_cast<std::size_t>(fsize));

        ErrorIf::condition(nread == -1);
        b.end = static_cast<char*>(b.end) + nread;
        fsize -= nread;
    }

    std::memset(b.end, 0, sizeof(char32_t)); // ensure null termination
}
Slurp::Slurp(const char path[]) : Slurp{FileDescriptor{path, O_RDONLY}} {}
Slurp::Slurp(const std::string &path) : Slurp{path.c_str()} {}
Slurp::Slurp(const std::filesystem::path &path) : Slurp{path.c_str()} {}

Slurp::~Slurp() {std::free(b.begin);}

const char *Slurp::begin() const noexcept {return static_cast<char*>(b.begin);}
const char *Slurp::end() const noexcept {return static_cast<char*>(b.end);}

std::size_t Slurp::size() const noexcept {return end() - begin();}

indi, first of all, thank you for putting so much effort into your answer. Even if I don't agree with everything you said, I really appreciate the work you put in.

The convention in C++ is to put all the type information together

I don't think there's any particular "convention" in C++ (or C for that matter). It's my personal preference/style to put sigils (*, &, &&) next to identifiers and not types.

Rather than holding two pointers, it might make more sense to hold a pointer and a size.

Again, this is more of a personal preference thing.

I also don’t get why [the begin_ and end_ pointers] are void*

Two reasons, first because the file may have any or no character encoding, so void* is semantically appropriate (whereas char* implies ASCII-compatible text and unsigned char* or std::byte* imply an unstructured stream of binary data), and second because posix_memalign(3) (which, as you say, may be overkill) takes a void**, to which a char** would need to be reinterpret_cast, and I don't want to open up that whole can of worms.

This [deleted default constructor] is technically unnecessary

Good point.

if your intention is to disable copying, then [the deleted copy constructor and copy assignment operator] are malformed.

I don't believe so, but it's not worth looking into this just to save a few characters, so I'll write these your way from now on.

you might consider making the type movable.

Done.

Writing functions that look like they take arrays like this is a really bad idea.

This is a low-level I/O routine, it's not written for beginners. While it does go back to a really bad language design decision on the part of the creators of C, I don't think you have to be an "expert" to know that foo(const char bar[]) takes a pointer. I think it's more semantic to write *bar when bar points to a single object and bar[] when, as in this case, it points to an array.

I am flattered you think I'm an expert! :)

As a general rule, all constructors that take a single argument should be marked explicit (unless you have a damn good reason not to)

I like explicit too but in this case I don't think there's any need for extra ceremony if a class has an operator char*/const char*.

In addition to this constructor, it would help usability if you could use strings or—even better—actual paths with this class.

Done, and part of what I intended to do anyway.

gnu::const on non-static member functions

I don't think your interpretation of gnu::const is correct. The compiler knows when a new object has been created, even if it shares the same address as a previous object. But it's a moot point since as per your suggestion I made Slurp movable, and so removed gnu::const from its member functions.

Never, ever [using namespace std;] at file scope. No, not even in a non-header file.

Opinions on this differ, I don't think it's that big a deal in a non-header file, particularly a short one with few includes, but I'll remove it anyway.

Avoid macros.

I was targeting C++11 in the original question's code, and wanted to ensure these were inlined. Since I'm targeting C++20 in this code I can and have made them constexpr functions instead.

But even that’s not really great, because it just obfuscates the function’s logic. I don’t see how it’s really an improvement

I guess it's a bit of a Ruby-ism (Don't Repeat Yourself) that I tend to do in other languages also. When I see repeated logic I extract it to a function (or macro as the case may be).

If you really don’t want all that ugly C-ish code, the right move is to properly wrap the API…

Maybe, but I'd say that's out of scope for now.

This is a really useful class! Rather than burying it as an implementation detail, it could be part of a public API.

Done.

You’d need to flesh it out a bit (and make it non-copyable, but perhaps movable)

I don't think it makes sense for a file descriptor to be movable; a POSIX file descriptor is conceptually unique, immutable and identifies a single object which is opened and closed exactly once (per file descriptor).

To make sure you get the right function, you should explicitly qualify it.

Done, although IIRC the POSIX standard may allow for some of these functions to be implemented as macros, in which case this may lead to compilation errors on certain (hypothetical) platforms.

The only thing I can figure is that you’re imagining that maybe someone might possibly want to reinterpret_cast the char const* to char32_t const*, and treat the data as a UTF-32 NUL-terminated string.

Yes, that's what I'm intending.

And it’s even odder when you consider that the Slurp class doesn’t even model a NUL-terminated string in any sense.

It was intentionally bare-bones for explication purposes, as you can see it's more full-featured in my updated code.

You are aligning the memory according to file_statistics.st_blksize… not alignof(char32_t). In other words, if you’re thinking that you can just reinterpret_cast the result of begin() to a char32_t const* and treat it as a UTF-32 string… you may be in for a nasty surprise when your program crashes due to a a misaligned read.

Well I'm assuming (perhaps strictly incorrectly) that st_blksize (512 on Linux) > alignof(char32_t) (4), and I'm pretty sure any allocation function (posix_memalign(3) included) will always allocate memory that's at least suitably aligned for std::max_align_t.

And all this is assuming the endianness of the UTF-32 data is correct.

Probably something I should look into, though I haven't addressed it in the below code.

if you want a regular string, why not read it directly into a std::string?

Because it introduces overhead and I intend for this class to be as efficient as possible. It's trivial to construct a std::string (or std::u32string, std::vector<std::byte>, etc.) from it if desired.

Honestly, I don’t see the point of [using posix_memalign(3)].

You may be correct, I need to write some benchmarks to see what the performance of posix_memalign(3) is compared to malloc(3).

I thought posix_madvise() was only for memory-mapped memory.

I don't think so? The man page says nothing about that. Again I should probably write a benchmark.

read() can fail spuriously for a number of reasons, like a signal interrupting it, and end up doing only a partial read. You can’t just write a read() call like this; you need to call it in a loop and keep doing reads until you’ve read fsize bytes in total

Good point, and done.

I mean… why not just std::fill_n(end, 4, 0);? It would probably be faster. (Unless your optimizer is smart enough to transform that memcpy() into a memset(), in which case it will probably be equally fast.)

Good point re: memset(3). In this ("C with classes"-ish) context I prefer it to std::fill_n().

Reading directly into containers

I largely agree with you, but I'm not doing so for three reasons: 1) Part of the point of this exercise was to be as efficient as possible, and reading into a container creates overhead. 2) Slurp is supposed to model any possible character encoding (or lack thereof) that a file might have. 3) C++17 and C++20 eliminate much of the need for this (at least for newly-written APIs) with std::basic_string_view and std::span.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.