17

p = malloc(n*sizeof *p); This is dangerous if n gets large, because the multiplication could overflow. After the overflow, too little memory has been allocated but code will continue without detecting the error. This is especially dangerous if n comes from untrusted source, such as some file format or remote user. Then it gives attacker easy way to ...


13

Minor stuff ... Allocate to the object, not type The below is easier to maintain. // char **aux = malloc(sizeof(char *)) char **aux = malloc(sizeof *aux) // aux[0] = malloc(bufsize * sizeof(char)); aux[0] = malloc(sizeof *(aux[0]) * bufsize); Avoid Exploit Below code is undefined behavior is the first character of user input is the null character. It ...


11

Looking at the performance, the two versions should perform just about identically. The second version has one less call/return, which can save a couple of CPU cycles, but if you have it multiple places in your code the additional code bytes and cache misses can overshadow that. Either way you probably won't notice a difference. Looking at readability and ...


10

In C, initialization and destruction should be done at the same level of abstraction. This is important because it defines who is responsible for the memory. There are two good ways to follow this guideline: Allocate and deallocate in the API's init/destroy functions (your first code example). fopen does this although it maps files rather than regular ...


10

Quick Review An API like this, dealing with thread-sensitive operations, requires time and effort to test and review rigorously. When I will find this time, I will do a thorough review. But here are some things I notice right off the bat. CommMemoryDisposedException should inherit from ObjectDisposedException. This way, consumers can handle your exception ...


10

So, you basically rewrote the strtok function in ANSI C. The difference is that you allocate memory for each substring while TOK modifies the original string by adding \0 characters in the place of delimiters. This means that you keep allocating more and more memory, while you can just make a copy of the whole string and use strtok to modify the copy you ...


10

Here are some things that may help you improve your code. Use the required #includes The code uses std::string which means that it should #include <string>. It might compile on your machine because some other header includes that file, but you can't count on that, and it could change with the next compiler update. Use only necessary #includes The #...


8

You've taken an expression statement and unnecessarily made a do/while statement out of it. You need parentheses around your macro parameters. You don't need to pass in the pointer you're assigning to as a pointer. Put all that together and you end up with: #define ALLOC(p, n) ((p) = malloc((n) * sizeof *(p))) This puts fewer restrictions on how it is ...


6

Expanding on my comment; below you'll find a very simple program that compares the method I suggested with your original example. The results on my machine show that the MemoryMarshal class is about 85x faster. You might want to experiment a bit and try running a similar test with a larger struct; maybe your method is faster for the specific problem that you'...


6

Review points: removeFirst on an empty list should be a no-op, (or throw an exception). last was dangling. Setting things to null of the removed first node, is not done in OOP, is left to the garbage collection. This also removes the need for an extra variable. And the resulting binary code is smaller. So: public void removeFirst() { if (first != null)...


5

noexcept on the explicitly defaulted default-ctor and copy-ctor is already implied. Copy-ctor and dtor would be implicitly declared if left out. .allocate() (and also .deallocate(), but that doesn't matter) fails to check n * sizeof(T) for wrap-around. Possible Solutions: Start with a check. This has the advantage of simplicity and broadest conformity. if (...


5

It is misleading that the same field in Link is sometimes referred as key and sometimes as value. LRU::insert works too hard. It calls findPage, followed by access, which in turn calls findPage with the same argument, and deleteKey, which also traverses the list looking for the same key. Three traversals with the same argument seems excessive. Consider ...


5

Command="$3" for (( i=4 ; i<="$#"; i++)); do Command="${Command} ${!i}" done # … ${Command} & Much cleaner to just reference the end of the $@ array directly. Since you're running the commands later, and would like to keep quoting intact, use an array: declare -a Command=( "${@:3}" ) "${Command[@]}" & # Break if the process has stopped ...


5

You must definitely call handle.Free() when finished using it so GC can do the cleaning. Note that GCHandle.Alloc(objects, GCHandleType.Pinned); only works for structs with pure primitive or to be more precise: blittable types. String fields etc. must be handled differently. A little optimization: You repeatedly calculate this var offset = Marshal....


5

Review You always instantiate inner dialogs for both platforms, even though one will never be called. Your platform will not change at runtime. Use the Lazy pattern to only instantiate the required dialog at first access. Rather than returning the magic path null, I would opt to use a TryShow method returning a boolean whether a path got selected an an out ...


5

Missing headers <cstddef> for std::nullptr_t <type_traits> for std::is_base_of_v, std::is_same_v, std::enable_if_t <utility> for std::exchange, std::forward Layout The code is hard to read, with long lines and huge blocks of spaces (perhaps a misguided attempt to align keywords?). Stick to a conventional layout and it will be much ...


4

There are a couple questionable things here, first since ALLOC() is a macro you don't need to pass the address of p. If ALLOC() were a function call then you would want to pass the address of p. Second, for an array I would probably use calloc() rather than malloc().


4

This is a general C code review, not addressing functionality. Overall the code is fairly well-written so most of my remarks are minor nit-picks. Could do with more comments. Your coding style is a bit exotic but as long as you keep it consistent, that's ok. Program design You don't seem to have an actual API for the functions. Even if they are supposed to ...


4

Warning messages When we print fixed strings, it's better to use plain fputs() rather than the much more complex fprintf(). However, in this case, the diagnostic output should be removed: such side-effects are not part of the contract of memset_s(), and are actively harmful (because the whole point of the checks is to report errors to the calling program, ...


4

In the first case, the caller is not given any control over allocation. This limits freedom and (therefore) performance: there is no control over the number of dynamic allocations or over which memory is used for what purpose, and there are limits on how the handle to the buffer can be stored (the returned pointer to Buffer must be kept around somehow, even ...


4

One class per file, and each with a dedicated header, is a pretty heavy-handed attempt at getting novices to modularize. As it is often the case, it is also counter-productive in this case. Link is an internal implementation-detail of LinkList and derived classes, aside from unfortunately and inadvertently being exposed by .delete_at_pos(). Fix that one ...


4

I would allow the default constructor and make it default to a 0 size array. with the optimization that b_array is a nullptr. Then you can also add move constructors/assignment. Where the moved-from object gets a b_array = nullptr; On of the basic functionalities of a container is iteration. So you should add a begin() and end() that return the b_array and ...


4

The default-ctor won't be implicitly declared as there are user-declared ctors. When you can define a default-ctor with reasonable behavior, consider doing so. If you use in-class initializers to 0 resp. nullptr for the members, it can even be explicitly defaulted, making the class trivially default-constructible. Top-level const on arguments in a function ...


4

Unnecessary code There is no need to make size, nemb even values. if(size % 2) size++; if(nmemb % 2) nmemb++; __get_next_allocation(size_t size) may benefit with rounding up of size Say you want allocations to be a multiple of 8. #define ALLOC_MULTIPLE 8 int __get_next_allocation(size_t size) { if (size % ALLOC_MULTIPLE) { if (size > SIZE_MAX - ...


4

Instead of flattening arguments into the string $Command, I'd suggest leaving these arguments in $@ by using shift: MaxMemory=$1; shift MaxTime=$1; shift # Command is now in $@ (Alternatively, read $1 and $2, then shift 2 for the same result.) We can produce output using portable printf instead of non-portable echo -e; it's probably a good idea to send ...


4

Review property Interval and methods ExternalStart, ExternalStart should throw ObjectDisposedException if _isDisposing is true property Interval and methods ExternalStart, ExternalStart should also acquire a lock on _syncObject when implementing the dispose pattern make sure to include a destructor ~DisposableSafeTimer or seal your class _isDisposing should ...


4

Additionally to the great answer of dfhwze, I'll provide an alternative implementation that includes some of the suggestions. Primary, I would implement each case separatly and create / dispose the actual dialog each time the method Show is called: public interface IFolderPicker { string Show(); } public class VistaFolderPicker : IFolderPicker { ...


4

The only thing I see that is a mistake is the bool operator. Instead of: operator bool () const noexcept You probably want: explicit operator bool () const noexcept The explicit prevents the compiler from being able to automatically convert your object to bool as part of a comparison. res_ptr<X> data = getResPtr(); if (...


4

Unless this is just for learning, always remember that adding yet another smart-pointer will seriously hinder interaction with anyone expecting a different one. The Design: The maker-function is mandatory: Cannot use a different allocator. Cannot generally be used to allow sharing across DLL boundaries, as DLLs (in contrast to SOs) don't do symbol ...


3

packagetarget_close() First of all, you have a copy-and-paste error in packagetarget_close(), where you attempt to free target->min twice, but not target->max. Next, note that most of the if statements are superfluous. As per the standard behaviour for free(), If ptr is a null pointer, the function does nothing. packagetarget_open() You have ...


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