10

Answers to your questions Am I using std::unique_ptr correctly here? Using std::move and get? Yes, you are using those correctly. Is there any way for me to make my _insert and _delete functions iterative? I'd like for it to be something similar to my search function. Sure. An iterative solution would indeed look like your search function. However, the ...


7

You have a number of problems with memory management in this linked list. The key thing to remember is that unique_ptr indicates ownership of an object. Use of release, reset, and to a lesser extent get are a code smell: not always wrong, but often an indication that the class is being used incorrectly. Usually you should use swap and move-assignment instead....


7

Your code produces no memory leaks since you're not manually allocating and deleting memory. Include all headers Always include all the headers. In this case, you need <iostream>, string, and cstring. Pass by const reference You should pass your objects by const reference. For example, const std::string& buf and const PacketType& packet. You ...


7

Here are some things that may help you improve your program. Use all required #includes Part of the interface is the #include files that are required, so those should be included here. Make your code hard to abuse Right now we're playing fast and loose with the assumption that memcpy will do the right thing, no matter what type is used for the template. ...


6

General stuff The implementation as-is does not attempt to be thread-safe in any way. I guess this is intentional, but might cause problem and/or confusion if not communicated well. There is no std::weak_ptr-like analogue. Might be a new challenge to implement later ;) Move constructor (and assignment operator) would be nice for SharedPtr. I was first ...


5

Am I going too far/extreme trying to place data onto the heap? Yes. Containers like std::unordered_map and std::forward_list already store contents on the heap. If you declare a std::unordered_map on the stack, only a little bit of administrative data goes on the stack. But the same happens with std::unique_ptr. So in your code, the use of std::unique_ptr ...


5

Your code is not correct. It can crash. First, look at the line fgets(buffer, BUFFERSIZE, stdin); If an error occurs while reading, the state of buffer is indeterminate. You should not continue processing it in that case. You need to detect this by the appropriate means and signal a failure. Second, if the malloc() fails, what will be the effect of the next ...


5

Overview Templated classes "normally" should provide all the code at the point of usage. As a result you should probably not have a separate shared.cpp. Though some people put the definitions in a separate shared.tpp file that is included by the header file. Code Review: You are missing a constructor for nullptr. Though nullptr can be converted to ...


4

I am not sure how it works at all as it shouldn't compile. Generic templates require the whole implementation in .hpp/.h files and nothing hidden in .cpp files aside from some possible instantiations. You have a ton of memory leaking due to ref_cnt{new RefCount()} in lots of places. Those are incorrect ways to write copy assignment/operator: ...


4

Move class Node and struct __iterator into class List It is very weird to see Node::Node<T> inside the code. A Node is an implementation detail of your List, so it should be declared inside class List. The same goes for __iterator. For example: template<typename T> class List { class Node { T data; std::unique_ptr<Node> ...


4

Data members I don't see the point of the T** ptrToPtr member variable. Every instance of *ptrToPtr can be replaced by ptr. Remove that member and you have one less thing to delete. Why is refCount a pointer to a long? If you make is just a long then you don't need to delete it. I see what's happening. Every instance that has the same pointer needs to have ...


4

Keeping objects as sequential as possible After reading the comments, it seems the most important use case is for tracking moves of objects in containers, but we want to keep those objects sequential, and be as cache-friendly as possible. It is also likely that you don't want to track all the objects in a container, but just a few. In that case, your ...


3

Code Review Don't see the point in this member: T** ptrToPtr; rCPtr() { try { ptr = new T; Calling new to create a new T is not appropriate here (having an empty object is perfectly fine). As the user may turn around and set a new pointer to own. As a result you don't need a try/catch block on this one. I don't mind ...


3

The fundamental problem is that the mechanism as it is very non-thread-safe. If you hold identifier in one thread and move it in another thread then it results in data racing as you don't use any memory synchronisation routines. Furthermore, you cannot move the object when another thread uses it. And currently there is no way to even test it if it is being ...


3

Your formatting is off at some places, use an automatic code formatter. BAD_PLACEMENT(-2), X(-1), EMPTY(0), O(1), SOLVED(2), RESET_GAME(3); You're mixing state of a single box, global game-state and state of input. Ideally you wouldn't. @Override public String toString() { ...


3

Because you don't intend to copy/move/assign SDL2_Fonts you could formalize that with SDL2_Font(const SDL2_Font& other) = delete; SDL2_Font& operator=(const SDL2_Font& other) = delete; (the move versions of these operations won't be generated if these two are defined or deleted). However, let's assume that one day we will want to be able to ...


3

Code Review The main purpose of the shared pointer is to prevent the leakage of pointers. Thus as soon as you hand the pointer to the shared pointer object, it becomes responsible for it and you must ensure that it is deleted. Here in the constructor that means you must protect against failures in the constructor. Note: If the constructor fails to complete, ...


3

When you have a C callback you usually pass two things A pointer to a C function A void* pointer that is passed to your C function. The C function must know the type of the original object so it can cast the pointer back to its original type. only after it has been cast back can it be used. And you must cast it back to the exact type it was before you ...


3

Adding to what Rish wrote, don't use std::endl. Just use \n in the string to indicate a newline.


3

Design/code review There is nothing to review in the first section… because there’s just no code there. The only comment to make is: virtual void common_action() override { /* ... */ } Use virtual OR override… not both. (Actually, the rule is “use only one of virtual, override, or final… but you should almost never use final.) So the only thing to really ...


3

The details of TrieNode don't need to be visible outside of Trie, so I recommend making it a private (or protected) member type Trie::Node. We don't need to write a constructor, if we initialise the root node with the value we want: std::unique_ptr<TrieNode> root = std::make_unique<TrieNode>(); Some coding styles ask us to indicate when auto ...


2

Your function is mallocing a value, and then overwriting it with a pointer to a local array, and then return the pointer to local array, which doesn't exist after the end of the function. This may work in certain cases, not work in others, and crash in others. This line doesn't copy the array, it changes the pointer. You probably meant for this line: ...


2

I can't comment on variable names because I don't know your language, this will limit the scope of the review I can do and I hope you understand everything in this review / answer. The memory management is good, I don't see any memory leaks. Test for Possible Memory Allocation Errors In modern high level languages such as C++, memory allocation errors throw ...


2

Having derefs is OK, as others mentioned in comments. I just wanted to address the existence of explicit derefs. We can use implicit derefs. This requires splitting off our Instruction type into JumpInstruction and AccInstruction, which is common practice, done for example in Rust's AST. We have: pub enum Instruction { Jump(JumpInstruction), Acc(...


2

Your idea to wrap the result of SDL_RWFromFile in a smart pointer is a good idea. I recommend a few improvements: Factor out the opening-and-wrapping operations into a named function. For example: auto OpenFile(const char *fname, const char *mode) { return std::shared_ptr<SDL_RWops>(SDL_RWFromFile(fname, mode), SDL_RWclose); } if( saveFile.get() ...


2

A really useful idea - well worth creating. Missing #include <memory>. With that fixed, I get almost clean compilation: 255367.cpp:9:5: warning: ‘LazySharedPtr<int, int>::init’ should be initialized in the member initialization list [-Weffc++] I agree, we should use the initialisation list for init: LazySharedPtr(Args ... args) : init{[args ...


2

There are different opinions on this: public String asString() { String rtn; switch(value) { case -1: rtn = "X"; break; case 0: rtn = "-"; break; case 1: rtn = "O"; break; default: rtn = "...


2

Tolerate NULL during clean-up As free(NULL) is OK, consider a NULL test for clean-up void cleanup_shared_ptr(shared_ptr_t * p){ if (p) { .... } } Lack of documentation *.h files deserve some overall comments. Consider users may not to wade through the .c file code to deduce functionality. Fixed width I see not compelling reasons for int64_t here. ...


2

The problem is line *(words+j) = &str;. It says that we want words[j] or *(words+j) to point to where the str is pointing. So, at the end of the while, all the words are pointing to the same str with the content of red. It can be solved using strcpy method from string.h header, which copies the values not simply change the reference. strcpy(*(words + j), ...


2

I learned to use space for programming instead of tab, and we were told to do 3 spaces, how many space do you recommend 2, 3, 4, or X amount? And how many space do you use when programming? This is a style issue and best to follow your group's coding standard which apparently is 3. I use 2. A good coding environment allows you to change the indent on the ...


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