New answers tagged

0

Some minor nitpicks about the constructor: It should be constexpr. It shouldn't have a default argument. We should check that the argument is valid (not negative, and not greater than max()). The standard says it's undefined behavior otherwise, so we don't technically have to, but it's nicer to assert than to silently carry on.


1

The only thing missing that's present in std::latch is the max constant. We can easily add that: static constexpr std::ptrdiff_t max() noexcept { return std::numeric_limits<std::ptrdiff_t>::max(); } I think that wait() is flawed: the first thread to get here (taking the mutex) will busy-loop (while). Really, we should wait on a condition ...


1

To make this even compile, I needed to add several standard-library includes: #include <any> #include <functional> #include <iostream> #include <map> #include <stdexcept> #include <string> I also needed to add the std:: namespace prefix in many places. (Don't be tempted to using namespace std - especially in your header ...


1

Use bool instead of int Consider changing the static flags to bool: static bool debug; static bool addNewLine; static bool doNotAddLine; Having both addNewLine and doNotAddLine does not make sense. Actually, since you are not using any of these flags you can simply remove them. Multiple constructors Instead of having multiple constructors, you could have ...


0

This code is nicely presented, which is great for reviewing. It would have been nice to see some usage examples, or (even better) some unit tests. One of the things that makes it hard to write good unit tests is that we've chosen to represent a file; if we make our core code work with std::istream and std::iostream, then it will be more testable, and we ...


4

A minor point in the main() function: you have neglected to define uint8_t. I guess that should be std::uint8_t from <cstdint>, and that's leaked into global namespace from one of the other library headers - but it's not portable to rely on that!


8

Overall, your code looks very good. No need to overload for empty template parameter packs A typename... parameter pack also matches zero elements, so the following overload is unnecessary: template <> struct TypeList<> {}; Use std::conditional to avoid having to repeat code You can use std::conditional to choose a type based on some condition. ...


0

Here, we're using an enum solely to select between different implementations. If we're not using it for anything else, then it may make more sense to pass a class object that provides those implementations. We start with an abstract class that defines the interface: #include <vector> class metric_space { public: virtual double distance(const std::...


3

In addition to G. Sliepen's answer: Return Type std::count returns a difference_type (i.e. std::ptrdiff_t). std::transform_reduce returns a std::size_t (because that's what we specify as the "init" argument). The other two return statements return 1; and return 0; are integers. So with the auto return type, the user will get a type depending on ...


3

Comparing using == is fine In the base comparison part, not sure using input == target as the equivalence condition is OK because ValueType and T may be different. This is fine, it does exactly what std::count() does, and I wouldn't know how else to compare two values for equivalence, apart from passing a predicate function, but then it would become a ...


2

When running your code (after changing from millisecond to native resolution on the timing) I get the following: clang++ -O3 Test# memcpy Avg std::copy Avg simpleLoop Avg pointerCopy Avg OMPCopy Avg 0 626.327 626 40.8343 40 59.0428 59 58.9443 58 58.962 58 1 41....


2

Answers to your questions Is there anyway to eliminate the global variables? Yes, you can put them on the stack of a function, like you already did in your second version, or alternatively you can make those variables static member variables of the class. yet keep the code as flexible and at the same time not repeat the code? I don't really see any ...


3

General points Why does compareDNA return a double when the inputs are both ints and the output thus can only ever be an int. The same function compareDNA seams out of place. This can be a method of a person. In rewrite see Person.compareDNA Avoid magic numbers. Use a constant to give number (strings, whatever) meaning. eg const int MAX_DNA_DISTANCE{2}; ...


4

Documentation Add documentation. I had no idea what the goal of this program is, going in, and at the end of the review I still don't, so I can't comment on whether the code is correct or efficient. Do we want to output the most likely child for a father? The most likely father for a child? What does "singleParent" mean? Often in this context it ...


1

It would be good to include some tests with the code, so that we can confirm it returns the correct results. It's very hard to read, because it is written like a C program, and fails to take advantage of the rich standard library provided with C++. We've reimplemented std::remove_if(), but we could just use that algorithm directly, for much simpler code. ...


0

A few minor things: return(false); etc. Don't put extra parens around the return value. return is not a function call. It is good that it looks different from a function call since it stands out better as a different kind of thing. When you start using templates and implicit types (like auto) and automatic move (rather than copy) of local variables, it ...


0

I have mainly worked on the ObserverID part. I agree with you on one point, it is rather artificial, when we know that C++ functions are hashable, so you could use the hash as ID. A point we must be aware is that std::function objects are not hashable (do not ask me why...) or at least I cannot hash them but others could not either For that reason, I would ...


1

Using gettimeofday may give you more precision than clock but is not likely to be any more accurate. When doing benchmarks, we are not so much concerned with how long a function takes in nanoseconds, but more with how long functionA takes compared to functionB. Consider the following plain vanilla C code: #include <stdio.h> #include <stdlib.h> #...


2

You are including several C headers that have C comments on the lines, and then several C++ headers. Don't use C headers. Sometimes, don't use the C library stuff in C++ (e.g. printf; you are also using cout! and this is a small piece of code! Is it just pasted up from fragments of C and C++ of different ages?) See Cppreference on C compatibility headers: ...


1

In addition to the issues coderodde pointed out, ⧺I.13 Do not pass an array as a single pointer (includes pointer and count parameters in the discussion) ⧺R.14 Avoid [] parameters, prefer span ⧺F.24 Use a span<T> or a span_p<T> to designate a half-open sequence Real reusable code would allow any kind of sequential container of values to be ...


1

Cannot tell about how to make your solution run faster, but I have some suggestions for the cosmetic issues in your code: size_t for counting stuff Instead of using int values for counting, it is more customary to use size_t instead. Reducing scope int numberOfTimes(int array[], int n) { int count = 0; int flag = 0; int sizeCounter = 0; while ...


1

WndProc setup It's been a while since I've done Windows API stuff, so my memory's kinda fuzzy, but I think there's an issue with how the window procedure is set up in the Window class: CreateWindowEx only returns after various calls have already been made to the window procedure. Until it returns, hwnd isn't set properly, and this isn't passed to the ...


0

There's a lot of repetition here - consider refactoring the timing code so you don't need to write so much to add timing (perhaps pass a lambda to suitable timing function). This kind of code: for( i = 0; i < amount; ++i ) { x1 = testme[i] / 2; } will be optimised away completely by any decent compiler, since the results are never used (the ...


2

for(int ctr=0;ctr<nums.size();ctr++){ if(nums[ctr]%2){ odds.push_back(nums[ctr]); }else{ evens.push_back(nums[ctr]); } } Well, you are repeating nums[ctr] three different places. Using the proper looping construct takes care of this automatically: for (const auto x : nums) { Now you already have the value in a simple named ...


1

Don’t write using namespace std;. You can, however, in a CPP file (not H file) or inside a function put individual using std::string; etc. (See SF.7.) I'm afraid the high-resolution-clock might not work right as it's not a monotonic clock. Which clock is best for this purpose depends on the platform, I'm afraid. And, I don't think any clock will be good ...


11

Avoid new and delete There is often no need to use new and delete manually in C++, containers do their own memory management, and in almost all other cases std::unique_ptr can be used. In your program, src and dst can be made std::vectors: std::vector<int> src(length); std::vector<int> dst(length); For the vector of test cases, you can use std::...


6

I think your approach is overcomplicated. You don't need to create/free objects and maintain a collection of objects. You just have the various different implementations that have the same signature. So give each implementation a different function name, and write: test ("memcpy", &TestMemCpy); test ("simple loop", &...


7

avoid manual memory management int* src = new int[length]; for (int i = 0; i < length; i++) src[i] = i; int* dst = new int[length]; We could use two std::vector<int>s for this and avoid the manual memory management. Pointers to the underlying storage can be obtained using the .data() member function. std::vector<TimedTest*> tests; ...


3

std::ostream&operator<<(std::ostream&ost,const std::vector<int>&input){ It's not just style. Peopledontwritelikethisbecauseitsreally reallyhardtoreadthisiswhatyourcodelookslikedontdoit. ... Please use a reasonable amount of whitespace. Being able to read the code easily is a huge step towards understanding it: std::ostream& ...


8

You know, C and C++ are different languages. You used both tags, and your code is confused as to which it wants to be. It appears to use C syntax, library calls, and ways of doing things; but it includes a C++ header. It doesn't seem to use that header though. Maybe this is the current state of the file after you have done some experimentation and ...


4

template<class T, std::size_t N> constexpr const T& stl::array<T, N>::back() const { return *(_items + N); } bug: Shouldn't this be accessing element (N - 1)? (Same issue with the non-const version). Otherwise everything looks pretty good. It's just nitpicking below: T _items[N ? N : 1]; I think this works (allows zero-sized array ...


0

The fastest way is to use memmove() and memset(), because as library routines they should be optimal. But if you can't do that for whatever reason, your best speed-up is from moving to using pointers. Looping over arrays with indexing is never optimal speed-wise (although it has obvious advantages for code readability). And since you're shifting by a ...


1

The code doesn't check if n is within the expected boundaries of the array. Unintentional overwriting of memory is such an infamous concern in C/C++, even in toy code I would add the range check to build good habits, or at the very least document in a comment that the caller is assumed to be trusted.


1

#include <iostream> #include <stdlib.h> #include <stdio.h> If this is C, don't attempt to include <iostream>. If it's C++, use the C++ headers (<cstdlib> etc), and prefer <iostream> to <stdio.h>. There's almost never any need for an implementation file to be bilingual C and C++ (sometimes it's useful for a header)....


2

Answers to your questions Which version is preferable? The latter has less dependencies, but the former has no raw loops... If having less dependencies matters, then go for the raw loops. If not, then I don't think it matters much; the lambda in the first version makes it just as verbose as the second version, and they should be functionally identical. Is ...


7

Why do you have the following line if you are not using temp? int temp = arr->A[0]; You do not need the nested loops. You are currently shifting the elements one position at a time when you can move them n positions. void LeftShift1(struct Array* arr, unsigned int n) { for (unsigned int i = 0; i < arr->length; i++) { if (i + n < arr-...


8

You can use memmove() function to move overlapping areas of memory and memset() to clear the moved items. LeftShift can be implemented like: #include <string.h> void LeftShift(struct Array *arr, int n) //n is the number of shifts { memmove(&arr->A[0], &arr->A[n], (arr->size-n)*sizeof(arr->A[0])); memset(&arr->A[...


3

Formatting: Don't be so stingy with the indentation, give your code some room to breathe! Validating input: You should check if std::cin was successful. It is always good if you can split out complexities into functions. Your main function does it all, read input, calc result, and write the output. I would suggest you created a separate function to determine ...


2

Generally good. A couple of weaknesses: We probably want to exit if std::getline() returns a failed stream. We might want to do the same for an empty input. Personally, I'd probably prefer to accept the message as command arguments, rather than reading standard input. On platforms without clear or where clear doesn't accept -x, we'll likely get an ...


6

Normally I do a design review first, then review the code itself. This time I’m going to switch it around. Code review Before I get into review this code, I just have to say that the sample solution code given is… ghastly. Seriously, if someone worked at a software company I owned, and they submitted that kind of code, I’d not only fire them, I’d have them ...


1

Binary Tic-Tac-Toe You can greatly simplify your code by representing moves as bits. Rather than have vectors of vector and all the loops in loops and indexes needed to search for patters bit field collapse all the logic into very simple math operations. 9 bits per player There are 9 positions so each player needs only 9 bits to represent there current moves....


2

Unnecessary use of using You are publicly inheriting from MatrixBase<...>, so there is usually no need to use using Base::... to explicitly bring member functions and operator overloads into the derived class. The only reason to do this is if you are providing an overload of such a member function in the derived class with different arguments types, ...


3

In your memoization you cannot distinguish between target sums that have not been evaluated, and ones that you have evaluated but have no solution. This results in re-running these unsuccessful possibilities. You should add some way to tell these two cases apart: Either use an empty vector to indicate that there is no viable sum, or use a class that stores ...


1

Node(string id ={}, bool isNum=false, int level=0, string data={}, bool duplicate=false, Node* next={}) Put the defaults in the argument list. That way you can leave off any number of trailing arguments. With Pepijn Kramer's answer, you only have two choices: give all the parameters or give exactly 2 parameters. You would need to overload 6 different forms,...


2

For just upgrading the char* arguments to object types, you should use string_view instead, and this does not require recopying the data. That's true for parameters in general. std::wstring_convert<std::codecvt_utf8_utf16<wchar_t>> converter; The meaning of wchar_t is implementation-dependent. I understand that on non-Windows platforms it's ...


0

Design: I'd recommend going the UTF-8 Everywhere route instead. This means avoiding std::wstrings and using std::strings in your program, encoded as utf-8. This requires a little care when processing strings, since you can't assume that one byte is one character, but it's better overall (see the link above for details). On Windows, you then need to convert ...


1

This is not as portable as we'd like it to be. Instead of assuming that the arguments are supplied as UTF-8 bytes, we ought to use the local character encoding, as used by std::locale{""}.


0

Don't use default arguments, but provide various overloaded constructors like this #include <string> class Node { public: // pass string by const reference, this avoids unecessary copying of the string // pass all other arguments as const, initializer is not supposed to change them Node(const std::string& id, const std::string data, ...


1

I "feel" symmetries could be leveraged using references. // to do: doc comment, decent variable names, access, const correctness TreeNode* rotate(TreeNode* x, TreeNode *&y, TreeNode *&y_child) { const TreeNode* r = y; y = y_child; y_child = x; return r; } TreeNode* rightRotate(TreeNode* x) { return rotate(x, x->left, ...


1

Don’t write using namespace std;. You can, however, in a CPP file (not H file) or inside a function put individual using std::string; etc. (See SF.7.) TreeNode(int key) { this->key = key; } Use the base/member init list in your constructor, rather than assigning values in the body. So: TreeNode(int key) : key{key} {} Where'...


Top 50 recent answers are included