Hot answers tagged

31

Since you already have a std::vector<std::string>, it's much simpler to let that own the memory, and build a parallel std::vector<char*> which just keeps pointers into the original strings. The only difficulty is ensuring this isn't used after the owning vector goes out of scope. The simplest implementation is something like: std::vector<...


18

I can see a few potential problems here: Since you allocated a char* array of input.size() elements, result[input.size()] is out of bounds. Similarly, std::string's size() is the number of characters - it doesn't include the trailing \0 needed for C-style strings. So every strcpy here risks doing a buffer overflow (risks, because it is possible for C++ std::...


13

Different approach The approach in the question is somewhat procedural. My aim is to use RAII to facilitate ease of use, as the code currently can leak memory and relies on programmer to free it. Usage cases Lets first have a look at the usage: Create cstring style array from std::strings. Sink the created array into exec family of functions Wait until ...


11

Firstly, there's a memory leak possible, because strlist performs two allocations using new[]. If the first succeeds, but the second throws std::bad_alloc, then we have no reference to *result with which to delete[] it. The answer by Incomputable shows how the interface can be greatly improved. I have a minor point that might be overlooked when reading ...


9

Overall this is some very nicely written C, well done. Some stuff I noted aside from @Morwenn's good points: Portability: <mqueue.h> is a POSIX C library. Unfortunately this restrains the platforms that you compile this for (I couldn't compile this on my Mac without some fiddling around). There are two ways you could fix this: Include the header ...


9

Generally speaking, the code looks good. It seems to be clean and to achieve what it is meant to achieve. Since your Makefile was given, as well as the compiler, I will review your code in function of what is legal with the -std=gnu89 compiler option, which means POSIX C89 + parts of C99 + some other compiler extensions. Here is a list of the allowed ...


9

do ... done is a compound command; every subcommand shares the file descriptors; so teeing the the loop has the same effect as teeing each subcommand. Two subsequent invocations of echo can be combined together. cat $file is a dreaded UUOC. A basename invocation can be avoided by changing directory to $FOLDER. ls is absolutely unnecessary. The shell already ...


9

Looks pretty clean for the most part! Here's the source of the actual GNU implementation used on most systems for comparison (if you already haven't taken a look at it). Just a few notes: Looks like part of this function could be converted into a for loop: void readable_fs(double size, char* buf) { const char* units[] = { "", "K", "M", "G", "T" }; ...


8

Use the C standard library Your code could be greatly simplified if you used the C standard library functions fopen, fclose, fread, fwrite and fprintf instead of the low-level POSIX system calls. The standard libraray also provides strlen which obsoletes your ft_len function but if you use the functions mentioned earlier, you won't have to count the length ...


7

You really need to reduce the scope of all your variables, and give them proper names. As it is, your code is impossible to follow, it's super hard to guess the type of anything, treacherous to determine whether it's been initialized properly or not, or whether it needs to be cleaned up. Discovering unused or redundant variables is pretty much impossible too....


7

These are good questions! 1. You should indeed have the copy constructor & assignment operator deleted. The "rule of five" tells you to specifically define a copy c'tor and assignment operator - but it doesn't tell you that you have to make the available. It is a perfectly valid choice to decide to not allow your object to be copied or non-move-...


6

Your description of the stack you wrote is incorrect. It does not handle char data; it handles char * data. Moreover, your functions can only operate on that one defined instance of the stack (s). This is BAD! Let's look at how we can fix this: Use Opaque Types One issue with the way you have your structure defined in the header file is that it exposes it'...


6

Memory leaks Since this is part of a shell and can therefore be considered an extension of the OS which should not stop at any point, you have a memory leak problem. Unlike Java and Python there is no garbage collection, you have to manage the memory on your own. Any malloc() calls you make either explicitly or implicitly with functions such as strdup() ...


6

2018/04/24 edit I missed an obvious bug: is_linux_mint is not implemented correctly. The results of a return on an empty output of a command substitution, with or without quotes, is implementation-defined: The value of the special parameter '?' shall be set to n, an unsigned decimal integer, or to the exit status of the last command executed if n is not ...


6

My thoughts for improvement: You don't need to run ToArray() on your string to loop through it. Plus, it seems like CalculatePermissionSum() is expecting a single permission string like "rwx" or "r-x", so in a fixed-length situation like this (where the flags are also in a predefined order), don't bother with a loop. int octalPerm = 0; octalPerm += (...


6

I find in general your implementation is a good start because it separates splitting the string into batches from calculating the octal value. You are not doing it in a single method. Using a switch is also not such a bad idea here but it lacks the case-insensivity that should be achieved with char.ToUpperInvariant If your application is unaffected by ...


6

There are some implicit problematic assumptions... You must not forget to call del_strlist, and call it exactly once. You clearly have to transport some data via unknown means from strlist call to the point where del_strlist takes place, namely the size of the vector. The user has to know how each string was allocated. As your char** data structure is read/...


6

There's one major problem with the code: Your move assignment is broken. You forgot to close(_s) before overwriting it with other._s. To avoid this kind of problems, I suggest using the copy-and-swap idiom. It makes writing a operator= a no-brainer in most cases: tcp_socket &operator=(tcp_socket other) noexcept // Note the lack of `&&`. { ...


5

I have a few comments. You use isatty to check whether standard output is connected to a terminal, regardless of what stream you're writing to. This means the rest of the functions only work correctly if you pass std::cout as the stream to which they're going to write. Otherwise, you may allow formatting when writing to something that's not a TTY, and you ...


5

This looks pretty good over all, but I have a few things that jumped out at me. Bugs structure_dir_entries has a leak and likely a crash in it if BUF_SIZE actually overflows. You're basically doing this: void f(char* buf) { buf = malloc(1); } void use_f(void) { char* b = NULL; f(b); free(b); } It looks like you need to check for absolute ...


5

Range in readable_fs(). readable_fs() should handle even larger sizes - consider future growth, so expand to K,M,G,T,P,E,Z,Y. Ref Pedantically there is no safe guard with double size not being some number like 1e100 and thus over-filing char buf[10];. To limit only local code that understands the limitations of readable_fs(), make it static and/or abort ...


5

Well written code. Good job. One thing I'd change is to instead use some meaningful name for each flag, instead of the flag letter. I don't recall by memory what -U does in the command, for instance, and so seeing a using_U in the code will evoke the same issue. You might remember the meaning of each flag right now since you have just implemented it, but I ...


5

Using a string for MOUNTPOINTS means you can't have any mount points with spaces of glob characters in the names. I realize you likely can't use an array if you want real portability but that's a severe limitation that you might want to check for. I don't know what you can do (without using an array) that will allow directories like that to work but you can ...


5

Broken If you call enterLock() with id 0 followed by enterLock() with id 1 (on another thread perhaps), both calls will succeed. Since your function doesn't change the state of the mutex, it isn't surprising. Perhaps you meant to leave lock->waiting[id] at 1 instead of 0? Note, even if you did that, it wouldn't be enough to fix the mutex. Also, what ...


5

Mixed use of array indexing with unsigned and int. size_t should be used. Rather than *pString1 = "\0";, *pString1 = ""; is certainly sufficient. Miss-guided comment. There is no NULL-ness in the array - just some uninitialized values in matrix[row][]. Could use calloc() instead. /* returns an array of char*, all of which NULL */ char **alloc_argv(...


4

posix_fadvise conveniently provides a POSIX_FADV_SEQUENTIAL macro. Use it instead of a magic 1 and a comment. The client doesn't know in advance whether the file is malformed or not. Better detect a malformed text file in run time: if (begin != buf_end) A Bugs section of posix_fadvise man page says that In kernels before 2.6.6, if len was specified ...


4

Broken for more reasons Without even looking at the details, I can tell that the mutex is not safe. The compiler is allowed to re-order your instructions which means that your writes and reads may not occur in the order you have written them. The compiler may actually even omit them totally... Not to mention there is no guarantee on memory ordering ...


4

Comments are not by any order of significance. Which operating system standard? A platform supporting C++ does not necessarily have an execve() call; that depends on <unistd.h>. Are you sure you're not making assumptions about what it contains? Perhaps you aren't, but you need to double-check and document this. Wrapping a single operating system ...


4

Get rid of unnecessary nested and combined ifs While here it goes to no extreme, it's advisable to check for errors first, and to do that one by one - if possible, which is exactly this case. Do some extra checks Only in case of 0 arguments, the script should output usage message. This script does not support multiple arguments, so if given more than 1, ...


4

I would not be impressed with this submission. Here are some of my notes: Way too many data type conversations. You're converting from string to array and back several times. No validation that the value is correct. I think you actually have an error in the permission string in your example. The format is well-known and you could use a regular expression. ...


Only top voted, non community-wiki answers of a minimum length are eligible