Hot answers tagged

16

This looks pretty good. I'd propose the following. First, you use set_value in exactly one place. It's a pretty unnecessary function, and just this would suffice: FlagSet &operator|=(const T &val) { bitset.set(static_cast<utype>(val)); return *this; } Second, for &=, the expression expr ? true : false is an antipattern that can ...


14

Layout Put your main method on the top, since it tells main purpose of the application. And, C# isn't like C++, you don't have to define the method ahead, before calling them. class Program { uint bitSize, shiftCount, mask, partionSize; static void Main(string[] args); void setValue(ref uint var, uint k, uint i, uint val); uint getValue(...


14

Don't write a function to return a constant You don't need uint_size(), as the returned value never changes (for a given target platform). You can replace it with #include <climits> static const std::size_t uint_bits = CHAR_BIT * sizeof (unsigned int); Don't require a call to init() The constructor should fully initialize the object; just move the ...


10

Overall your code is very well formatted and readable but I have some complaints at the moment: Don't declare all methods with throws Exception. Only declare the Exceptions you really need to declare, such as IOException. Declaring throws Exception will force the caller to also catch all RuntimeExceptions, which I say is a big no-no as that could ...


10

IndexOf can probably be a nested type of FlagSet, unless you intend for that to be part of the public interface. using underlying_t = std::enable_if_t<std::is_integral_v<U>,U>; Are you sure SFINAE is what you want to do here? It would probably make more sense to static_assert that U is integral. At the same time, you might also want to static ...


10

<bits/stdc++.h> (like everything in your compiler's bits/ subtree) is not a standard header and therefore not portable. Even if you're willing to sacrifice portability, it's a poor choice, as it will slow compilation down compared to simply including what you use. using namespace std; is poor practice. It makes your code less clear, and it may even ...


8

After studying your code for a good long while, I realized that the reason you haven't gotten an answer is that there isn't really much to say about your code. Well done! I would recommend adding some thorough ScalaDoc though. It has been my experience that Scala library developers have far too much faith in the ability of library users to magically ...


8

I have a number of comments, so I'll just sort of spray them out. Using a proxy reference type is tricky, and can lead to some interesting problems. For example, a class that provides proxy references (e.g. vector<bool> cannot meet the standard's requirements for a container (see http://www.gotw.ca/publications/mill09.htm). It also makes the use of ...


8

I agree with your assessment of the code being unnecessarily long and awkward. It looks like the root cause is an opportunistic optimization. The bad news is that most of the secondary tests (like arr[mid - 1] == 0 and arr[mid - 1] == 1) are bound to fail, so not only they contribute to awkwardness of the code - they also hurt the performance (by about a ...


8

not directly answering your question, but just a notice: I would say the more elegant way would be just extending the std::bitset and add the extra methods you need, for ex. to_bytes(): template <size_t N> class bitpattern : public std::bitset<N> { public: using byte_t = uint8_t; static constexpr size_t num_bytes = N/8 + ((N % 8 == 0) ? ...


7

Accumulator Strategy One minor optimization can beat the single processing in a long run (\$n >= 100000000\$). Instead of update the directly AtomicSet for each non-prime number, you can update an intermediate variable with same size of each chunk of the AtomicSet and using this intermediate value to update the chunk once. // now, mark all ...


7

Prefer typed constants over #defines: NO_OF_BITS should be a const unsigned int or const size_t and should also be a private member of BitArray, since it is not used elsewhere. Also, NO_OF_BITS is a bad name for the constant. Num of bits in what? Num of bits in an integer. So named it accordingly. As it was already suggested, use CHAR_BIT from <climits&...


7

This removes the learning part of this (if your goal is to try to learn to implement a binary search in this context), but I contend the most pythonic solution would be to use bisect.bisect_right. It gives you the rightmost insertion point for a value via binary search. The rightmost insertion point for a 0 would be the number of zeros, so you just subtract ...


6

In addition to the other answers: Be more direct towards the user The template parameter n is not very explanatory, n pieces of what? In your case I can deduce it to be number of int that should back the array. As a user I don't care how many ints are backing the array, in fact it's also a portability issue as an int can be anything from 16 bits and up. ...


6

The code is relatively short and well-written, so I didn't find many nits to pick. With that said, here's my review: Include all needed files Since std::ostream_iterator is declared in <iterator> that should be among the includes in main: #include <iterator> Use appropriate type for template parameter The std::bitset template is defined as ...


6

You're not the first to be frustrated at the bitmasks we've inherited from K&R - it would be really nice if the language provided better support in this area, and many libraries have given us different takes on the answer. Qt uses its moc preprocessor to turn Q_FLAGS enums into QFlags<E> value types with suitable operations, for example; that's ...


6

The test for enable_if_all_same_as_T seems a bit over-restrictive - surely all the arguments only need to be assignable to FlagType, not exactly the same? That's achievable with an initialiser-list constructor: constexpr explicit Bitmask(std::initializer_list<FlagType> args) noexcept; Yes, that means you no longer get to play with fold expressions, ...


6

Here are some things that may help you improve your program. Use all required #includes The templated class uses CHAR_BIT but is missing this line that provides the definition: #include <climits> Eliminate unused variables The variable input in several tests in your code is defined but never used. Since unused variables are a sign of poor code ...


5

You are using an std::vector for temporary storage inside push_back(). This is a possible point of optimization, since the size of it is constant ((N + 7) >> 3). You could use an std::array in this case to make sure no dynamic memory is allocated. If you are concerned however that your N is going to be, in some cases, big enough to cause a stack ...


5

Micro-Optimizations One very micro-optimization you can make is, instead of subtracting numbers, add the negative variants. For example: var stopIndex = vector.BitLength - 1; Is faster as: var stopIndex = vector.BitLength + -1; It's just the nature of the beast. Subtraction takes more cycles than addition, so if you add the negative complements of your ...


5

When you post code to be reviewed: If your code ask for input, provide a set of valid an invalids inputs If your code output a result, express expected. Since you present your code, it may be interesting to embrace a coding standard. You do not pay taxes on written characters. Do not be afraid to use spaces to improve readability. Don't include <bits/...


5

Small portability bug: if we're using false in the macros, then bflags.h should include <stdbool.h>. Or use 0 instead of false. Definite portability bug: we shift a (signed) int before promoting: #define BFLAGS_WORD_MASK(bit) (1 << ((bit) % BFLAGS_WORD_BIT_SIZE)) I think we need the 1 there to be of the appropriate type first: #define ...


5

Most severe issue: Never invent secret macro languages! This is about the single-worst thing a C programmer can ever do, all categories. You are perfectly free to assume that any C programmer will understand what 1<<bit means. You cannot assume that they will understand what BFLAGS_WORD_MASK means. It's nothing but obfuscation of what would ...


5

A significant performance drain is bit-by-bit computation loops such as TypeBase<Size> &operator+=(const TypeBase<Size> &other) { bool carry = false; for (size_t ix = 0; ix < Size; ++ix) { data_[ix] = add_(data_[ix], other.data_[ix], carry); } return *this; } Unfortunately, at this time such constructs are ...


4

Power of 2 Instead of using: (int)Math.pow(2, numbers.size()) You can compute a power of two more efficiently like this: 1 << numbers.size() You can use 1L to compute it as a long if you think that the power may exceed 31. Manual bit manipulation Instead of using a BitSet to find the 1 bits in your int, you can use integer arithmetic to check ...


4

It looks like you want to optimize something but you did not define what are your performance requirements. Your tests do not justify your implementation. Why the state of builder is stored in array of longs? In toString method you rewrite everything to StringBuilder and build the String. This approach increases complexity of the whole class without known ...


4

There are some correctness issues with this code that need to be addressed immediately. There is no need to use pointers for the BitSlice(Mut). These should happily accept regular slices as the backing storage. Check if you are going to go off the end of the vector before doing it! The code for the slice iterators currently looks like this: let next = self....


4

It's a great idea to provide a type-safe bit-mask, I like your idea a lot. That said, I feel like your code is in great need of modernization and simplification. Recursion through inheritance is now a testimony to the ingenuity of our great predecessors, but it's time to get acquainted with if constexpr and fold expressions. I agree that you'll need C++17, ...


4

Don't declare multiple variables on a single line. it is error prone and more difficult to read. int n,q,k,count; should be: int n; int q; int k; int count; Not sure what I mean by error prone? int* n,q,k,count; How many pointers do you have? one. only n would be a pointer in this declaration. Let your operators breathe. The lack of whitespace makes ...


4

Here are some thoughts on how to further improve your program. Use all required #includes The program refers to CHAR_BIT which is defined in <climits> but only <limits> is currently included. Similarly, std::size is defined in <iterator>. Eliminate unused variables The idx is sometimes used and sometimes ignored. My thought is that ...


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