1
\$\begingroup\$

I'm writing my own "standard library". It relies on nothing but syscalls, so I've had to make my own implementations of everything, including getline (renamed to getl).

The functions used, though renamed to avoid annoying lint messages, should be pretty self-explanatory; zalloc is calloc but with malloc's signature, crealloc is realloc, and read is a thin wrapper over the bare syscall.

The function reads a line from stdin, using the following steps:

  • Allocates an initial buffer
  • Tries to read enough bytes from stdin to fill the buffer
  • If the buffer has been filled, allocate more space
  • If the newly read data contains a newline, break; otherwise, loop the previous 2 steps
  • Resize the buffer to use precisely how much space is required to hold the string

I'm not certain if the more conventional method (use space allocated by the user) is better for this situation; it seems like that would complicate matters. I'm also unsure if using an exit variable would be preferable to goto. Finally, I'm not sure if I should be throwing on an error. However, I'm very sure that I prefer the int* x style over the int *x style, no matter how unconventional; it makes infinitely more sense.

    char* getl() {
        char* buf = (char*)zalloc(8); // allocate zeroed space
        if (!buf)
            throw "failed to allocate space for getl";

        size_t i = 0;
        size_t size = 8;
        size_t used;
        while (true) {
            size_t oldi = i;
            size_t r = read(0, buf+i, size-i); // fill the buffer, if possible
            if (r == -1)
                throw "failed to read from stdin for getl";

            i += r;

            if (i == size) {
                size *= 2;
                buf = (char*)crealloc(buf, size); // increase the buffer size
                if (!buf)
                    throw "failed to allocate more space for getl";
            }

            for (size_t j = oldi; j <= i; j++) {
                if (buf[j] == 10) { // stop once a newline is reached
                    used = j;
                    goto eloop;
                }
            }
        }

    eloop:
        buf = (char*)crealloc(buf, used+1); // remove excess space
        if (!buf)
            throw "failed to resize space for getl";

        buf[used] = 0;
        return buf;
    }
\$\endgroup\$
5
  • 1
    \$\begingroup\$ Please provide the source for crealloc() and all the other memory allocation functions. Is there a reason you avoiding the use of new? Where is the catch() that handles the throw? Currently there is not enough code included to review. \$\endgroup\$
    – pacmaninbw
    Commented Jun 1, 2023 at 2:14
  • \$\begingroup\$ The link with the text my own "standard" library is the link to the Github repository. I didn't feel it was necessary to provide the source code for functions whose behavior is known, and whose backend is irrelevant. What throw is this catch supposed to be handling? This is a utility function in a library, not an executable. Where should I be using new? \$\endgroup\$ Commented Jun 1, 2023 at 2:40
  • \$\begingroup\$ I was under the impression that I shouldn't be asking for a review on multiple unrelated things in one question. The code I wanted the most feedback on was this, so I published this and linked the rest. There is clearly enough code to review, since many questions on this site have less. This is why so many people find Stack Overflow and related sites so hostile: you thought it was easier to post a snarky comment showing little to no understanding of simple concepts than to click a link. \$\endgroup\$ Commented Jun 1, 2023 at 2:45
  • \$\begingroup\$ I did click the link, but I can only review was is posted in the question itself. Links to repositories can be used as references but the code there can't be reviewed. Code Review is quite different from Stack Overflow, please read A guide to Code Review for Stack Overflow users and How do I ask a good question?. \$\endgroup\$
    – pacmaninbw
    Commented Jun 1, 2023 at 14:36
  • 1
    \$\begingroup\$ "Be sure to embed the code you want reviewed in the question itself; you can leave supporting, but non-essential, code in links to other sites." "Try to focus your code on to only those things you want reviewed. If you want an algorithm reviewed, then try to include only the code that is related to the algorithm. This reduces the amount of 'distraction', and it also means there is less code to read, and criticize." I provided the function I wanted analyzed the most (readl). I provided the required context to understand the function, and a link to the rest. I did my due diligence \$\endgroup\$ Commented Jun 1, 2023 at 16:33

3 Answers 3

4
\$\begingroup\$
  1. Don't paraphrase your code in your comments.
    Comments which aren't doc-comments are for explaining why you do or don't do something if refactoring the code is insufficient, and maybe giving a simplified overview of your algorithm if needed.
    Paraphrasing takes time, steals attention, gets stale in a hurry, and distracts from making or recognizing worthwhile comments.

  2.      char* buf = (char*)zalloc(8); // allocate zeroed space
    

    As the buffer will be overwritten anyway, why have it zeroed before?

  3.          throw "failed to allocate space for getl";
    

    Throwing pointers to string-literals is at best a stop-gap if you don't have your exceptions sorted out yet.

  4.                  if (buf[j] == 10) { // stop once a newline is reached
    

    Use a proper character literal, and the explanation becomes superfluous.

  5.                      goto eloop;
    

    Avoid goto. It is much harder to read than most other flow control, because it is much less restricted.
    In your case, this is the only way to leave the infinite loop, aside from exceptions, so why force the code below the loop?

  6. Don't throw away whatever follows the newline.
    It must really be left for the next request, be it by storing it in a buffer somewhere, not reading it (which is likely to be inefficient due to more syscalls), or rewinding.

  7. Why have i and used?
    They have the same semantics, though non-overlapping scopes of use.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for your suggestions! Throwing a string is indeed a temporary measure, as I try to decide between C-compatibility and ideal error handling. \$\endgroup\$ Commented Jun 1, 2023 at 17:56
3
\$\begingroup\$

This looks like an "off by one" error in your code here:

for (size_t j = oldi; j <= i; j++) {
   if (buf[j] == 10) {

Shouldn't that be j < i? Now you potentially read outside your allocated buffer.

\$\endgroup\$
2
\$\begingroup\$

Standard bad use of realloc() (or your crealloc()).

            buf = (char*)crealloc(buf, size); // increase the buffer size
            if (!buf)
                throw "failed to allocate more space for getl";

In realloc() if it fails to realloc the buffer then it returns null but it does not release the original buffer. Assuming your version does the same then if the function failed and returns null then you have leaked buffer.

The proper way to use realloc() is like this:

            char* newBuf = (char*)crealloc(buf, size); // increase the buffer size
            if (!newBuf) {
                // release the old buffer
                // so you don't leak it.
                throw "failed to allocate more space for getl";
            }
            // It worked. So you can now replace buf
            buf = newBuf;

Your are throwing a string literal:

      throw "failed to read from stdin for getl";

Here you are throwing char const* so you have to know this to catch that exception. Better to throw a more standard exception class.

      throw std::runtime_error("failed to read from stdin for getl");

But this is NOT the main issue with your throws. You are leaking memory.

    char* buf = (char*)zalloc(8); // allocate zeroed space

This is not a managed buffer. If you throw this is leaked. You either have to explicitly put this in a managed object std::unique_ptr or when you throw you must explicitly make sure the memory is released.


Goto!

                used = j;
                goto eloop;

The same effect can be achieved in so many more readable ways. Goto has it uses but abusing it can lead to very unreadable code. This is a slippery slope you don't want to step onto without a very good reason. If you have a good reason then make sure you add the appropriate comments to explain yourself.


My main issue. Though is you loosing data from the input stream after the 10 character. You have read it into the local buffer. You then scan the local buffer finding the end of the line:

        for (size_t j = oldi; j <= i; j++) {
            if (buf[j] == 10) { // stop once a newline is reached

BUT: This is not returned to the file stream. You simply return your current buffer (after resizing to drop the extra part of the string). So you loose the whole chunk from the input stream that you never put back so the next call to getl() will never see this input (it just magically disappears).

And since you use a size that increases very quickly this can be an arbitrarily large part of the input stream that suddenly disapears.

Nit Pick: It's more natural to express new line as '\n' rather than 10.

  if (buf[j] == '\n')
\$\endgroup\$
1
  • \$\begingroup\$ Thanks for pointing out my issues regarding memory allocation! I am working on fixing larger issues (such as this) first, so I don't yet have an exception type; however, in testing my allocator I did make an equivalent to unique_pointer, so I can use that. For the moment, I've implemented a read_stdin function and a buffer to fix the stdin issue; I will probably use a more long-term solution once I've got the more major bugs sorted out. \$\endgroup\$ Commented Jun 2, 2023 at 18:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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