Hot answers tagged

32

The algorithm looks correct. Regarding the function signature, I'd make three changes: You're not modifying elements, so take it by constant reference rather than reference. You don't need to specify a pointer to Function, Function is already a template parameter, and non-pointers could be valid (ex: a class with a () operator). "callback" isn't a ...


26

As mentioned, this is a re-implementation of std::vector, not std::list. The default constructor can be set as default. You're not following rule of 0-3-5. Consider this snippet of code: int main() { list<int> l1 = {1, 2, 3}; list<int> l2 = l1; } As things stand, your implementation is invoking undefined behavior because both l1 and l2 point ...


17

I think you did a pretty good job as a beginner. In addition to what Errorsatz said: Consider making element a const reference to prevent unnecessary copying: const auto& element : elements You missed #include <string> in the test file. std::endl flushes the buffer and causes performance degradation; use \n instead unless you need the flushing ...


16

In addition to Toby Speight's remarks, I would add: Use constexpr for all constants I see you already used constexpr for image_code and label_code, but you didn't use it for the static member variables rows, cols and labels of MNISTObject. But those look like they are constants as well. You can just write: struct MNISTObject { ... static constexpr ...


16

I'm mostly going to play advocate for the devil here. Lock-free doesn't mean fast There is a rather persistent misconception that lock-free algorithms are faster than locking algorithms. However, that may not be true. Modern mutex implementations are extemely fast in the uncontended case, and when there is a lot of contention they use a system call that lets ...


15

const bool Game::isColliding(... This indicates that the returned type is const, and should not be modified by any part of your code. But it's not a reference, so it doesn't make sense. It should compile, but it's a little confusing. Removing that const is clearer. see this SO question for more information on that. Your parameters to isColliding should be ...


14

Store Money as Fixed-Point Monetary quantities are the classic example of something that might have fractional values, but you do not want to represent as a floating-point number, because you need exact arithmetic, not fast approximations. At the same time, it’s a good idea to store at least one extra digit of precision, so that fractions of a penny will ...


12

First of all, the compiler is not hostile; by using -ffast-math you are asking for an optimization that the compiler dutifully performs. Perhaps it is not what you desired the compiler would do in this particular case, but please don't attribute that to maliciousness from the compiler or the compiler developers. Compilers routinely are able to look at code ...


11

SFML implements functionalities that allow you to determine if two rectangles intersect. You can significantly shorten your current code: const bool Game::isColliding(const sf::RectangleShape& player, const sf::RectangleShape& enemy) const { return player.getGlobalBounds().intersects(enemy.getGlobalBounds()); } See more: sf::FloatRect::...


10

John's answer is correct on programming matters. I want to comment on the logic of your code. I found the collision detection calculation difficult to interpret. Which point on the square is represented by playerPosition? The documentation for SFML says it's the upper-left corner (although in a roundabout way), but it could also reasonably be the center. I ...


10

That's a very big list of includes! It might be a sign that you have functionality that should be split out to separate source files (e.g. filesystem read/write away from the core computation). Keeping the standard library includes alphabetical is a good choice - it allows very easy checking of whether a needed header is already included. This looks broken,...


10

The front, back, and swap methods don't check for an empty list. You leak a lot of memory. Most/all of the places where you have this->elements = new you haven't freed the previous memory stored in this->elements, resulting in a leak. clear does not free memory. append reads one past the end of the memory block stored in previous because the loop that ...


9

Overview Your code as written is perfectly fine (apart from the one bug in subtraction). Anything mentioned here is mainly to help future readers. You overcomplicated the design by adding constructors to Vector2D. The default constructors work perfectly and as expected in this type of situation. There is an argument to make operators members of the class ...


9

template<class T, uint8_t BitsForItemCount> class RingBuffer This is missing #include <cstdint> using std::uint8_t That said, I really dislike headers that populate the global namespace, so I would prefer to see std::uint8_t (and its friends) written in full. Is there a reason we need an exactly 8-bit type, or would std::uint_fast8_t be a ...


9

While your algorithm is \$O(n)\$, the constant factors are considerable. Specifically, you are suffering due to all the nodes the map needs. Better options are: Just std::sort() and then do a single pass. While the order is \$O(n * log(n))\$, it will probably still be far more efficient. Use an array for a histogram. 256 elements (wrap-around of an ...


9

Use std::uint8_t instead of BYTE I see you are declaring a type alias BYTE. However, I think that is a bad idea. If you really wanted to talk about an opaque byte, then since C++17 there is std::byte. However, you cannot do any arithmetic with std::bytes. Instead, you want to treat bytes as 8 bit integers. There is already a perfect type for this: std::...


8

Non-conformance: [Derivation].4: All types specified in the C++ standard library shall be non-final types unless otherwise specified. std::vector is not an exception, and thus must not be final. You forgot to implement support for hashing by specializing std::hash. You don't add the necessary typedef to sub-namepspace pmr for polymorphic allocator support. ...


8

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{}}, ...


8

Naming things You have a bounds checking evaluation function, operator(), and one that doesn't do bounds checking named chebeval(). If the idea is to have this mimic an STL containers operator[] vs. at(), then perhaps make it more like that: have operator() do the unchecked evaluation, and have a member function named at() that does the bounds checks. ...


8

Toby Speight and G. Sliepen gave excellent feedbacks from the programmer's perspective; A friend of mine gave some feedbacks from the machine learning researcher's perspective, as follows: User should be able to specify hyperparameters (lr, weight_decay, etc), or the code should include hyperparameter tuning Weight matrix initialization is wrong. It should ...


8

Good job on general organization: use of namespace and nested detail namespace, use of different sized char types, marking things with noexcept, etc. Error: Why do you return false when the function expects a ptrdiff_t? Architecture: mixing function return values and "out" parameters or, worse yet, "in/out" parameters makes it harder ...


8

Not lock-free You've replaced locks with busy-waiting, but that doesn't make the algorithm lock-free. In TryConsume, the first consumer to win the race for the to-be-read item is then responsible for advancing the read pointer. Your other consumers wait for this advancement in a busy-loop -- making no progress. In order for the algorithm to be lock-free, ...


8

You are obviously on a fantastic quest to understand how to implement image processing operations in all sorts of languages. The tag reinventing-the-wheel tells me that you know that you should use an established image processing library as the basis of any serious work. Here are some comments on your C++ implementation. image.h Proper English spelling would ...


8

Using a std::unordered_map is overkill here, since the number of possible characters is known beforehand. So you could just use a std::array<std::size_t, 256>. But you can do even better than that: you don't need to know the exact number of occurences of a given character, you only need to know if it's odd or even. You just need one bit to keep track ...


7

Your naive solution is quadratic time, O(n^2) and O(1) additional memory. Note, polynomial time doesn't say much about the actual time complexity other than it's not exponential or factorial. Your improved version is O(nlogn) time for the implementation with set and O(n) for the one with unordered_set and both come with the additional cost of O(n) memory. ...


7

General I found no "dumb mistakes associated with generic code". Good job. Still, there are some points which can be improved. (Aren't there always?) The first point is encapsulation. Letting nodes float around free, instead of keeping them in their dedicated container, or (rarely) evacuating them to a handle for re-insertion, is not recommended. ...


7

Avoid unnecessary copies enqueue() takes its parameter by value, which means the caller has to make a copy. Then it has to store that value in the array, making another copy. You can avoid copies by replacing the function that takes a value with two overloads, one for const references and one for rvalue references: void enqueue(const_reference_type val) { ...


7

Avoid restricting template types too much You are restricting the value type of the matrix to artithmetic types and complex numbers. But what if I had a custom type that implemented fractions, or had some other type for which matrix multiplication would make sense? If you cannot predict all possible valid types, don't attempt to restrict the template type at ...


7

This is looking very good. I especially like the improvement of submatrix(). Still, there are a few things I mentioned in my review of the 2D class that I'll repeat here, as well as add a few other remarks. Avoid restricting template types too much I see you are using std::regular now instead of concept Scalar. This still requires that the type is equality ...


7

Minor compile issues Not all compilers support coroutines fully yet, and at least on GCC 10 your generator fails to compile, because: gen.cc:70:16: error: the coroutine promise type ‘std::__n4861::__coroutine_traits_impl<generator<int>, void>::promise_type’ {aka ‘generator<int>::promise_type’} declares both ‘return_value’ and ‘return_void’ ...


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