Specifically, setEachNthBit - sets every Nth bit starting at a specified start bit operator= - assignment operator|= - bit or and assignment operator&= - bit and and assignment operator^= - bit xor and assignment
<rdar://problem/63481333>
Created attachment 399939 [details] proposed patch.
What do you plan to do with these?
(In reply to Sam Weinig from comment #3) > What do you plan to do with these? I'm exploring implementing a MarkedBlock FreeList using Bitmap instead of a linked list of FreeCells. For that purpose, computation of which block atoms are free is done using bit math on Marked bits, NewlyAllocated bits, and some other mask. The setEachNthBit() method is needed to make the other mask.
Comment on attachment 399939 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=399939&action=review r=me, with a couple of suggestions for the tests. > Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:1001 > + { Is this block just the same as the one above except for s/size/smallsize/g ? If so, is it possible to make it an helper function to avoid the repetition? > Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:1093 > + temp.set(i, (randomNumber() > 0.5)); I am not sure that a random number in a test is a great idea. It sounds like a recipe for flakiness/non-reproducibility. > Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:1102 > + Bitmap<smallSize, WordType> smallTemp; Same comment as above, maybe try merging the tests for smallSize and for size with a helper function. > Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:1113 > + smallTemp.set(i, (randomNumber() > 0.5)); see my comment above, I think it might be nicer to bake somewhere (at the top of the file?) an array with the random-looking values, instead of drawing them truly randomly at each execution. > Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:1427 > + // 1 & 0 typo: & instead of ^ > Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:1434 > + // 1 & 1 ditto. > Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:1506 > + // 1 & 0 ditto. > Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:1513 > + // 1 & 1 ditto.
Comment on attachment 399939 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=399939&action=review >> Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:1001 >> + { > > Is this block just the same as the one above except for s/size/smallsize/g ? > If so, is it possible to make it an helper function to avoid the repetition? Not identical because of the different data sets used. Also I don't have predetermined data sets for the smallSize case. So, I'm using randomNumber to generate them in some of the cases below where needed. I can look into refactoring this. >> Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:1427 >> + // 1 & 0 > > typo: & instead of ^ Thanks fixed.
Created attachment 399981 [details] patch for landing.
Comment on attachment 399981 [details] patch for landing. View in context: https://bugs.webkit.org/attachment.cgi?id=399981&action=review Thanks for improving the tests! > Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:29 > +#include <wtf/RandomNumber.h> No longer needed.
Comment on attachment 399981 [details] patch for landing. View in context: https://bugs.webkit.org/attachment.cgi?id=399981&action=review >> Tools/TestWebKitAPI/Tests/WTF/Bitmap.cpp:29 >> +#include <wtf/RandomNumber.h> > > No longer needed. Will remove.
Thanks for the reviews. Landed in r262030: <http://trac.webkit.org/r262030>.
This introduced a nice GCC warning spam: In file included from DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-f0a787a9-2.cpp:5: /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.cpp:194:16: warning: implicitly-declared ‘constexpr WTF::Bitmap<4>::Bitmap(const WTF::Bitmap<4>&)’ is deprecated [-Wdeprecated-copy] 194 | return result; | ^~~~~~ because: DerivedSources/ForwardingHeaders/wtf/Bitmap.h:480:13: note: because ‘WTF::Bitmap<4>’ has user-provided ‘void WTF::Bitmap<bitmapSize, WordType>::operator=(const WTF::Bitmap<bitmapSize, WordType>&) [with long unsigned int bitmapSize = 4; WordType = unsigned int]’ 480 | inline void Bitmap<bitmapSize, WordType>::operator=(const Bitmap& other) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ To avoid the warning, we should either (a) remove the copy assignment operator, or (b) add a copy constructor. Either way would be fine, but in this case I'd prefer (a) because I don't think the new copy assignment operator is needed. It uses a loop to manually copy the bits of the sole member variable, a std::array, but a default copy of the array would be equivalent, right?
(In reply to Michael Catanzaro from comment #11) > This introduced a nice GCC warning spam: Created: bug #212421