Stack Exchange Network

Stack Exchange network consists of 175 Q&A communities including Stack Overflow, the largest, most trusted online community for developers to learn, share their knowledge, and build their careers.

Visit Stack Exchange

New answers tagged

1

Don't import the standard namespace Namespace std is a large, and growing, namespace. Do you know every identifier in it? Including the ones to be defined in C++20 and beyond? Bringing all its names into the global namespace not only eliminates the benefits of using namespaces, but also has the potential to silently and subtly change the meaning of your ...


3

gen and dist don't need to be pointers. Just declare them as members, and use the member initializer list in the constructor to initialize them. std::random_device rd; std::mt19937 gen; std::uniform_int_distribution<T> dist; EasyRandom(T a, T b): gen(rd()), dist(a, b) { } In the operator() that returns a vector, you can reserve space for the vector ...


2

Stop using namespace std; Having using namespace std;, especially in a header file is considered bad practice. This Stack Overflow post explains the reasoning quite well. In your case it's especially unnecessary since you do not even use something from this namespace in BST.h and only std::string in Packet.h. Unnecessary includes Again BST.h. There are a ...


2

I like your clear variable names (though I'd prefer Amount rather than abbreviating to Amt). Avoid using namespace std - it's a big (and growing) namespace that's not intended for wholesale import like that, and doing so can unexpectedly change the meaning of your program. std:: is intentionally very short, so get used to typing it. Don't use std::endl ...


3

I'll start the serious bug, then work inwards from main(). Biggest bug The biggest problem with the code is that it only reads one command from each connection, leaving any subsequent commands for the next client to execute. Even if we read more commands, by looping until handle() returns false before closing the client socket, this still won't help if ...


3

When looking at code like this, the first thing that comes to mind is the Single Responsibility Principle Every function module should be responsible for doing one thing. In this case, you can start with main calling the function that starts the program. You can have the string literals that you want to print to the screen as string arrays and a function ...


1

Couple of points on improving the implementation: Line 7: futex_wait doesn't specify FUTEX_WAIT which should be the third argument. syscall(SYS_futex, addr, **FUTEX_WAIT,** block_if_value_is, nullptr, nullptr, 0); You should use FUTEX_WAIT_PRIVATE and FUTEX_WAKE_PRIVATE - more optimal unless you intend to use this mutex for cross-process synchronization. ...


2

A few short comments: Initializing a matrix with a compile-time list of values (initializer list) does not seem to be very useful - as CUDA is used to process huge amounts of data, not a tiny number of elements you provide at compile time. Mixing code for mathematical / data structure abstractions with error handling utility code into the same class is not ...


4

At first, I was surprised by the use of a class here, but having read the code (and especially the main()), it clearly makes sense. It might be possible to make it more natural to use by providing a (const) iterator to access the values, rather than the getCurrent()/getNext() interface. Consider defining the streaming operator in the implementation file. ...


5

Normally, we'd use the vector's index type (std::vector<int>::size_type, i.e. std::size_t) for the return values. But unfortunately we're required to return negative values when the search fails, so I'd recommend a pair of std::ptrdiff_t instead. And the question specifically asks for ints, so I'd just insert a comment explaining that were limited to ...


2

Although there's a comment in SimpleDestroy<T, false>::destroyElements() claiming that ~T() doesn't throw, there's very little to assure that. Our only guarantee is that we currently call it only from the version of SimpleCopy that handles non-throwing destructors, but it would be easy to accidentally break that requirement - for example, we might ...


3

One aspect that could surprise users is that operator & always narrows types to match. That's a departure from the usual guidance of "do what the integers do". On one hand, it makes sense not to waste resources on bits that will always be zero. One of my own use cases is to extract the some or all of the lower 26 bits from a 130-bit value, so it seems ...


6

I think this function is unnecessary. We can deal with defective classes (that don't properly implement the standard LessThanComparable concept) by either fixing them (preferable) or by providing a comparator argument to std::max: auto const b_lessthan = [](const B& a, const B& b){ return !(a>=b); }; std::max({B{}, B{}, B{}, B{}, B{}}, ...


2

Your code seems really nice to me. It is well structured, and the use of ranges and concepts increase its diagnosing ability. As I mentioned in my comment, std::optional is really a C++17 thing. You tagged your question c++14, so I will assume you want implement it under C++14. Here's some suggestions on the code. Your code is missing a trailing } to ...


8

Nice: using bytes = std::vector<std::byte>; I would call it Bytes to make it clear it is a type rather than an object. This is not a good idea: using namespace std; See: Why is “using namespace std;” considered bad practice? This is technically not valid C++. byte data[length]; Variable sized arrays (VSA) are an extension to the language ...


3

//since std::thread objects are not copiable, it doesn't make sense for a thread_pool // to be copiable. True. The default copy constructor would be ill-formed, so it is not emitted, so you don't need to manually disable it. Same for the assignment operator. It's even worse for std::mutex and std::condition_variable which cannot even be moved. You can make ...


1

I played around with this a bit more and came up with some alternatives. First I tried to use basic_ifstream<byte> like this: bytes read_block(uint32_t offset, uint32_t length, const string& filename) { basic_ifstream<byte> is(filename, ios::binary); istreambuf_iterator<byte> it(is); bytes data; ...


3

Why is there #pragma once in the implementation file? Including it twice in the same translation unit is clearly an error that should be fixed, not a normal state of affairs as a header would be. If the abs() is meant to be std::abs(), we need #include <cmath> and using std::abs;. The getMin() and getMax functions seem to be exactly like std::min(p, ...


3

Recently I came to the realization that I been holding myself back by doing this and moving beyond my insecurities. I have taken more leaps by writing common C++ programs to build my knowledge of the language and architecture. Learning by doing is a good approach IMO. Hopefully you can improve your game and submit it again for review! main Is Player.h ...


8

A few more suggestions in addition to the great existing answers: Support logging stack traces - it's easy! Standard C++ offers no facilities for obtaining stack traces, so traditionally - logging libraries and manual logging have foregone those. But these traces are extremely useful in inspecting logs and debugging programs (despite their verbosity); and ...


8

You have some good answers already. My criticism comes from one word. "printf". The problem for fault-finding is not just knowing what each part has reported, but also knowing in what order they happened. Any practical logging library is going to hit threads sooner or later, and at that point things go wrong. For starters, we need to think about thread-...


6

I'm not an expert in c++, so I will focus mostly on the correctness of your implementation w.r.t. the algorithm as described in the book. Numerical Robustness Often, the main challenge in implementing geometric algorithms is the fact we cannot work with the real numbers and therefore implementing geometric primitives (is this point to the left of a line, ...


25

Lots of good code comments already given. I'll focus on non-code aspects. Take this from a DevOps engineer that regularly troubleshoots complex, unfamiliar systems from logs under time pressure. You want all logs to always be consistently formatted and to have timestamp and location (source file and line) by default, it looks like you request the date with ...


13

Making functions in header files static means that each translation unit gets its own separate definition. This usually isn't what we want. Standard practice is to declare the function in the header, and then define it in a .cpp file, e.g.: // header: #include <string> namespace lwlog { namespace datetime { std::string ...


0

Couple of things I found myself so far. Performance wise Huge mistake, to use std::async for multiprocessing, while testing it on my local machine with only one CPU it worked okay, using all the cores, but when switched to test server with more than one CPU, I failed to use all of them (actually 32). Changing this std::thread , without any pain btw, did ...


20

I see a number of things that may help you improve your code. Separate interface from implementation The interface goes into a header file and the implementation (that is, everything that actually emits bytes including all functions and data) should be in a separate .cpp file. In this case virtually everything in datetime.h and all static functions in ...


1

Wrap your code into a namespace // this can be used to create a calendar! class TimePeriodGrouping Why not name the class something like CustomCalendar to indicate that it is a calendar but not a conventional one? Order your interface from public to private. Naming your member variable period when your surrounding struct is named Period seems like a ...


5

A few observations off the top of my head cerrMutex is unused, and should be removed if not needed. mapMutex is used to guard a static std::map but is itself a regular class member variable. That suggests it probably isn't guarding the map against concurrent access. This is probably a bug. This seems like a fairly heavyweight tool. The uses of maps and ...


2

Your code looks extremely nice and well-structured to me. It exhibits modern C++ coding idioms. You also include references to the standard in your code. All of these are greatly appreciated. Here are some suggestions: I like sorting #includes according to alphabetical order. Like this: #include <condition_variable> #include <functional> //...


5

I recommend a naming style that uses CamelCase for template parameters and snake_case (only) for plain old variables and functions, so that the reader can tell at a glance if something is a template parameter. For example, in the code that follows, you use indices as the name of a parameter pack; where I would name that pack Is (Is being the plural of I, and ...


2

Style Subjective but don't indent on namespaces. I would group the ctors and operators together instead of grouping them by their status (default/delete). Your ctor is a bit hard to read. Consider formatting it differently. E.g.: DirectoryWatcherWin::DirectoryWatcherWin( const std::string& directoryToWatch, const std::vector<std::...


4

Headers and namespaces Don't using namespace, especially a big and growing namespace like std that's not designed for it. There's no need to include <cmath>. On the other hand, there is a clear need for <iostream>, which has been omitted. Structure There's no need for the Power class; it maintains no state. The functions should simply be ...


3

/* ThreadPool class It is a singleton. To prevent spawning tons of threads, I made it a singleton */ ThreadPools can be useful as both singletons and not singletons. There is zero need to mix the ThreadPool implementation with the "this is a singleton" implementation. There is a lot of need to not; there are some nasty things you have to look out for with ...


3

This question is already quite old. Still there's a lot more to suggest for improvements. Here are some suggestions not mentioned yet. Don't use #define for constants: This: //read only variables - can be changed. #define NUM_OF_BOMBS 60 //HAS TO BE LESS THAN 100 AND LESS THAN HEIGHT * WIDTH #define HEIGHT 13 //the amount of actual "...


5

And another one for posterity ... Macros - just say No. #define MAX_THREADS std::thread::hardware_concurrency() - 1; don't put a semicolon at the end of a macro, it means you can't write resize(MAX_THREADS) do parenthesize your macros, like #define MAX_THREADS (std::thread::hardware_concurrency() - 1) so you can write resize(MAX_THREADS/2) don't use ...


5

The problem is that you are erasing elements from prog and then without bounds checking access it with prog[j]. That is sure to fall over. Your end_time function is commonly known as operator< So you should use the appropriate name. There is no comment on the significance of 1 and why a program should not be reordered when it starts at 1. It seems that ...


12

It's great that you provide a test program. Although it doesn't yet test very much, running it under Valgrind uncovers a few wild accesses: ==17803== Invalid write of size 4 ==17803== at 0x109773: Vector::resize(int) (221707.cpp:68) ==17803== by 0x10938A: main (221707.cpp:103) ==17803== Address 0x4d74c94 is 0 bytes after a block of size 20 alloc'd ==...


12

You tagged c++11, but your code is not at all C++11. You have no move constructor and no brace-initializers. You should implement them. Here's a sample implementation using the copy-and-swap idiom. friend void swap(Vector& a, Vector& b) { using std::swap; swap(a.capacity, b.capacity); swap(a.sizet, b.sizet); swap(a.arr, b.arr); } ...


10

Commenting You could improve the functions with some introductory comments. In particular, the isPrime() predicate has an extra argument compared with the conceptual version - we should be clear what that's for (i.e. it's an ordered set of all primes up to √num). Similarly, sieve()'s results argument is assumed to be empty, but that's not clearly ...


12

When you find a prime number, say 7, you begin crossing out all odd multiples of 7 from the primeCandidates vector. You do this by incrementing index += loop, where loop is the prime number, and but primeCandidates only holds odd candidates, so the step size in natural numbers would be 2*loop. The issue is you start by removing 7 from primeCandidates, then ...


7

First of all: great code. I'd love to read code like this in our implementations. For me that's a nice mixture of useful comments when needed with self-explaining code. I also like the modern way of writing using constexpr instead of #define, which I still see a lot. I'm not a C++ pro, rather coming from the C# side, so I notice the following: std::size_t ...


11

You've received good style advice so far. But your pool doesn't work. So let's try to address that. 1. 'resize' is both wrong and not needed. As others mentioned, it's not thread safe. Making it so is exceedingly difficult. And in practice, you don't need to change the thread pool's size after after start. 2. You only use one thread at a time. In ...


2

There are a lot of good comments here already, overall good work ... The resize function is not threadsafe, especially since you are using a singleton you don't know where this will be called from, not making this threadsafe leaves you open to race conditions. You're not unlocking JobQueue before the call to notify, that seems strange, the thread that ...


9

std::thread::hardware_concurrency() can return 0,you should handle this case.


14

Use better algorithms: Sieve_of_Eratosthenes You used a brute force algorithm. But even this can be highly improved. You increment by 2 each loop. for (int i = 5; ; i = i+2) { So you have noticed that all even numbers are not prime. You can improve on this. By incrementing by 2 then 4 then 2 then 4. This removes all multiples of 2 and 3 automatically....


3

Boost.Signals2 is a thread-safe library. It uses mutex locking internally. There are some caveats, as explained in this article: Almost all classes provided by Boost.Signals2 are thread safe and can be used in multithreaded applications. For example, objects of type boost::signals2::signal and boost::signals2::connection can be accessed from ...


14

@osuka_ and @anderas gave some very good advice. I just have a couple things I want to add: Macros #define MAX_THREADS std::thread::hardware_concurrency() - 1; While it doesn't really make sense to do arithmetic on this, macros like this should be surrounded by parentheses so the order of operations works correctly. Without it you get weird stuff like (...


4

Here are some ideas that may help you improve your code. Use functions Right now this code is all one big block of code in main. This makes it more difficult to read and understand than if functions were used. Simplify your code Consider this sequence of code to read two integers: std::string first; std::getline(std::cin, first); std::istringstream iss(...


8

@osuka_ has already provided a thorough review, but I want to show an important point that is missing from his review: The choice of making your class a singleton and the way that you implemented it. I suppose that you have a really good reason to make this class a singleton. But sometimes, the singleton pattern is considered an anti-pattern because it ...


13

You really need a better algorithm. Look into Sieve of Eratosthenes for a good first prime sieve to implement. That avoids (expensive) division, using instead only addition and simple multiplications (I'm including % as "division" here). In is_prime, you really only need to try dividing by the previously discovered primes, rather than by all numbers. If ...


Top 50 recent answers are included