dfaure added inline comments.
INLINE COMMENTS
> tfry wrote in krandomtest.cpp:178
> If the results are not unique, some of the results will already be in the
> set, and so results.insert() does not increase the size of the set. Only if
> each thread result is unique, the set size will correspon
tfry added inline comments.
INLINE COMMENTS
> dfaure wrote in krandomtest.cpp:168
> variable size array, which is not in the standard.
> https://stackoverflow.com/questions/1887097/why-arent-variable-length-arrays-part-of-the-c-standard
>
> Make size static to fix it.
Sorry. Should have waited
dfaure added a comment.
No - not with this syntax. See the stackoverflow discussion I posted, or
https://stackoverflow.com/questions/31645309/can-i-use-a-c-variable-length-array-in-c03-and-c11?rq=1
REPOSITORY
R244 KCoreAddons
REVISION DETAIL
https://phabricator.kde.org/D5966
To: tfry, d
aacid added inline comments.
INLINE COMMENTS
> dfaure wrote in krandomtest.cpp:168
> variable size array, which is not in the standard.
> https://stackoverflow.com/questions/1887097/why-arent-variable-length-arrays-part-of-the-c-standard
>
> Make size static to fix it.
They are in C++11, no?
R
dfaure added inline comments.
INLINE COMMENTS
> krandomtest.cpp:168
> +const int size = 5;
> +KRandomTestThread* threads[size];
> +for (int i=0; i < size; ++i) {
variable size array, which is not in the standard.
https://stackoverflow.com/questions/1887097/why-arent-variable-length-a
rjvbb added a comment.
I have no official reference for Qt requiring C++11, but I'm pretty sure I've
seen the remark made on a mailing list or one of their code review tickets. I
would have guessed that was with 5.7 but it can just as well be 5.8 and later.
A GCC >= 4.8 *probably* correspond
This revision was automatically updated to reflect the committed changes.
Closed by commit R244:26a262180155: Ensure proper per thread seeding in
KRandom. (authored by tfry).
CHANGED PRIOR TO COMMIT
https://phabricator.kde.org/D5966?vs=14863&id=14952#toc
REPOSITORY
R244 KCoreAddons
CHANGES
mpyne added a comment.
The changeset is fine to go in (once the duplicated semicolon is fixed).
As for C++11, I'm not aware that Qt requires C++11 as a blanket policy.
Instead they require specific compilers, and then we can use the C++11 features
supported by that compiler.
Accord
tfry updated this revision to Diff 14863.
tfry added a comment.
Switch to QThreadStorage, use quintptr as suggested, add auto-test.
REPOSITORY
R244 KCoreAddons
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D5966?vs=14831&id=14863
BRANCH
master
REVISION DETAIL
https://phabric
tfry added a comment.
In https://phabricator.kde.org/D5966#111878, @rjvbb wrote:
> but I also didn't verify if QSet::insert is thread-safe.
Ouch, you're right, it isn't. QThreadStorage it shall be, then (until we can
use thread_local). That also handles thread-destruction, properl
rjvbb added a comment.
Using QSet was my suggestion, if cheaper than QThreadStorage - something I
didn't check, but I also didn't verify if QSet::insert is thread-safe.
About C++11: since when exactly does Qt require C++11 again?
REPOSITORY
R244 KCoreAddons
REVISION DETAIL
https://p
mpyne added a comment.
Would it be better to use `QThreadStorage` (to record if the thread has
been seeded) instead of a `QSet`? It would allow us to more easily adopt
C++11's `thread_local` once we remove support for older compilers. I also
looked into whether we could adopt the higher-qu
tfry updated this revision to Diff 14831.
tfry added a comment.
Ensure proper per-thread random-seeding in KRandom::random().
As commented, the problem is somewhat different, than I though, originally:
qsrand() and qrand() keep random seeds per QThread, and each thread needs a
separa
rjvbb added a comment.
I was going to reflect that it would be better to set `init` in an atomic
operation, but:
In https://phabricator.kde.org/D5966#111735, @tfry wrote:
> On further investigation, I see that qsrand() would probably have to be
called for //each// thread, in any cas
tfry added a comment.
On further investigation, I see that qsrand() would probably have to be
called for //each// thread, in any case. So that would mean keeping track of
initialization state in a QThreadStorage?
REPOSITORY
R244 KCoreAddons
REVISION DETAIL
https://phabricator.kde.org/D5
tfry created this revision.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.
REVISION SUMMARY
Consider the following scenario: Thread A calls KRandom::random(), for the
first time.
It will now go through a (relatively) expensive proced
16 matches
Mail list logo
