Hot answers tagged

38

Well, first off, why do you use a std::vector for a comparatively small sequence of known length? A raw array or std::array suffice and avoids any dynamic allocation. Next, avoid needless magic numbers. Use std::mt19937::state_size instead of manually specifying 624. Why do you use a lambda? A simple std::ref(source) suffices. The seeding itself looks ...


38

Why do most people (on the internet) recommend using recursion because it's simpler and easier to write the program? Logically I thought that we should write it in a way that is fast and simple. This is a perceptive question. I wrote an article about exactly this topic in 2004, which you can read here: https://docs.microsoft.com/en-us/archive/blogs/...


34

I think it's very strange that you provide iterators and an operator[] for an IP address. Generally speaking, IP addresses are not considered to be "iterable"; an IP address is just a single address. If you were modeling a subnet mask, like 127.0.0.0/8, then it might make sense to model it as a range of addresses; but if you're modeling just a single address,...


31

There are several small things that you can improve: double weight = 0; works fine, but double weight = 0.0; would be more pedantic since weight is a double. By the way, the following line is a good example to illustrate the benefits of literals pedantry: double volume = ((4/3) * M_PI * pow(obj.radius, 3)); Here, 4/3 performs an integer division and not a ...


30

Prefer Reference over Pointer Since the producer and consumer must have a buffer you should pass it by reference (rather than pointer). This also makes sure there is no confusion over ownership of the buffer (the owner of a pointer is responsible for deleting it). By using a RAW pointer you can not tell the owner but by using a reference you are explicitly ...


29

Issues Your vector assumes that the type being stored has a default constructor. arr = new element[size]; Also this is really inefficient for expensive to create classes where you don't use all the size members. You want to design your class so that the objects in the vector are only constructed when the element is first added. Bug You don't implement ...


28

Design review Card Let’s start by looking at the Card class. As designed, Card has 4 data members: value suit name rank value is an unsigned int, while the others are all std::strings. This is enormously wasteful. A card really only has two properties: rank and suit. If you know the rank, you know the value and the name, so duplicating that data in the ...


27

For example, is it normal for the class to have at least one node? In other words, this implementation cannot have an empty linked list. The length is always 1. No, a linked list can be empty (i.e. size 0). But there are designs that intentionally always have at least one member in the list. This special member is referred to as a sentinel and should ...


26

Loki's solution does not enforce the order in which the function calls are performed, because the order in which function arguments are evaluated is unspecified. Here's a C++14 solution that ensures the function is called from left to right: #include <cstddef> #include <tuple> #include <utility> template <typename Tuple, typename F, ...


25

Profiling: We could use a library like Tracy to do the sort of profiling you mention in the question. But it's quicker to ask "what's taking the most time" and get an answer with a profiler like Very Sleepy, or the built in Visual Studio profiler. So... I stuck your code into a project created with create-vs-napi, and ran stringify on some json ...


23

As you are correctly assuming there are multiple threading related issues with your code, but lets tease you and start with the usual suspects. Naming The name apply_prime is misleading and inexpressive. Neither does the function really require a prime nor does apply do it justice. You should name it something along the lines: strike_out_multiples or ...


23

Readability Please put a space after the include and before the < #include<iostream> #include<string> #include<memory> Don't do this You can read any other C++ revue on this site. using namespace std; See: Why is “using namespace std” considered bad practice? Don't add useless comments // Template Node template <typename T> ...


23

Design The main thing about a vector is not constructing its members until they are put into the container. The vector you have constructs all the members immediately which can be very expensive if your type T has an expensive constructor (or you never use any of the members). T* storage = nullptr; As a result you don't want to use T* as your storage type ...


23

I'll hit the red flags first, and then review the details. template<typename T> const shr<T> make_shr(T * obj) { return shr<T>(obj); } "Returning by const value" is a red flag. It doesn't do anything except occasionally disable move ...


22

Firstly, it's a good first attempt at writing some threaded code. The major sticking point is that you're passing in an int & and returning void. Of course, std::thread will just run some code and won't return you a result. However, within the C++11 threading library, there are a number of things you can use that will allow you to return results, instead ...


22

Generally it's nice, well-structured code, but it relies on a promise that might not be kept. Specifically, simultaneous access to different elements in std::vector<bool> is not guaranteed to be thread-safe because storage bytes may be shared by multiple bits in the vector. Consider an alternative way to slice things. Each thread could be ...


22

Memory management new T[m_capacity] default-constructs m_capacity objects of type T in there. This might hurt performance a lot if Ts default constructor is not trivial, and even then it still isn't necessary. Even worse, in pushBack a new T object gets copy constructed in the place of the already existing one (if m_size < m_capacity), without properly ...


21

Extend your timer function so it can take all the parameters needed by func() Don bother to pass func by const reference. What I would have done template<typename TimeT = std::chrono::milliseconds> struct measure { template<typename F, typename ...Args> static typename TimeT::rep execution(F func, Args&&... args) { ...


21

You can make the class template with trivial changes (add template<typename T> and change int by T in your classes), then make a construction function that deduces integer types: template<typename T> LoopRange<T> range(T from, T to) { static_assert(std::is_integral<T>::value, "range only accepts integral values")...


21

Ok, so here we go for a few tips: Several methods such as AABB::contains and AABB::intersects do not modify the AABB instance they are called with. Therefore, you should const-qualify these methods to make sure that they can also be called in a const context. bool intersects(AABB other) const { /* ... */ } ^^^^^ As pointed by @...


20

Header files It's strange that this code uses the C header <string.h> but the C++ versions of <cmath>, <ctime> and <cstdlib>. I recommend sticking to the C++ headers except on the rare occasions that you need to compile the same code with a C compiler. In this case, I don't see anything using <cstring>, so we can probably ...


20

We use std::string without including <string> - a possible portability bug. Representation as a string of char isn't very compact, so we're quite wasteful of space for large numbers. The modifying operators (++, *=, etc) all return void, but it's normal and expected that they return a reference to *this (act like the integers). It's suspect to have ...


19

C style Well, the first thing I have to say is that your code is very C-like. The first action here would be to move all those loose functions and globals into two C++ classes, Lexer and Parser. Another C vice of your code is to declare variables on top of a function. lex() is champion at that, with 18 variables piled at the top of the function. Absolutely ...


19

This seems like a perfect opportunity to leverage the standard library. I would make a function to toggle a single character and then use std::transform on top of that. Not only is it more consise, it will be more performant (you should use reserve to preallocate the resulting string -- otherwise the string may allocate over and over again). #include <...


19

Overall Ohhhhh. Is it well-formed? It compiles, so yes. i.e. does it follow common C++ standard and patterns (for example, should private members be declared before public ones? Personally I think so. * Private Variables * Public * Constuctor / Destructor * Copy Semantics * Move Semantics * Swap * Other Public Interface * Friends *...


18

I'd rewrite it with three main changes: Keep main() minimal and improve the user interface. By the Single Responsibility Principle, it's a good idea to limit main() to just calling the primary function with the appropriate parameters. In this case, the functionality splits very cleanly. Being a Unix/Linux user, I would prefer to see tools that adhere to ...


18

I would have probably applied the following changes: Make size a static constexpr variable in CallerTraits instead of simply static const. Wherever a function simply passes variadic arguments whose types have been deduced, I would have passed args by universal reference (now officially called forwarding reference) and used std::forward to forward the ...


18

Design This is a very thin C like wrapper around sockets. In my opinion there are a lot of style changes that are need to make this good C++ or usable by modern C++ library. Main issues: Two phase construction Error Codes escape public interface (use exception) You member variables have state not related to the object after construction. Your use of ...


18

Multiple threads are usually used to compute things in parallel. In this example nothing is computed in parallel: while one thread is running, the other is waiting. With no practical value, it's not a great demonstration of multithreading. I suggest to look for more practical targets in the future. Since evenready and oddready always have opposite values, ...


18

If you had tagged this code as C, it would have been acceptable. Since you tagged it as C++, it's horrible. Instead of writing your own swap function, there's already std::swap in <algorithm>. Instead of writing bubble sort yourself, just use std::sort, also from <algorithm>. Instead of using arrays and resizing them yourself, just use std::...


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