Re: Review Request 126474: Port QRegExp to QRegularExpression in kshorturifilter

2015-12-22 Thread Ivan Čukić 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126474/#review89939
---


The patch seems straight-forward.

I can not find the reference that QRegularExpression is thread-safe (the most I 
see is 'all functions in this class are reentrant which is stated for QRegExp 
as well).

The QRegExp vs QRegularExpression situation is quite confusing, but the later 
should be preferred in Qt 5.x - it is faster if nothing else.

- Ivan Čukić


On Dec. 22, 2015, 4:52 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126474/
> ---
> 
> (Updated Dec. 22, 2015, 4:52 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> A static QRegExp was used but it is not thread safe. QRegularExpression
> seems to be.
> 
> BUG: 352356
> 
> 
> Diffs
> -
> 
>   src/urifilters/shorturi/kshorturifilter.cpp 
> 1af4768b7b5ab9d1f5af52f17170d466d854b9bb 
> 
> Diff: https://git.reviewboard.kde.org/r/126474/diff/
> 
> 
> Testing
> ---
> 
> I ran the autotests which includes urifilter and I've run krunner which uses 
> it extensively.
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126474: Port QRegExp to QRegularExpression in kshorturifilter

2015-12-22 Thread David Edmundson 


> On Dec. 22, 2015, 4:59 p.m., Ivan Čukić wrote:
> > The patch seems straight-forward.
> > 
> > I can not find the reference that QRegularExpression is thread-safe (the 
> > most I see is 'all functions in this class are reentrant which is stated 
> > for QRegExp as well).
> > 
> > The QRegExp vs QRegularExpression situation is quite confusing, but the 
> > later should be preferred in Qt 5.x - it is faster if nothing else.

you're right docs don't mention it, but see Alex's comment on 
https://git.reviewboard.kde.org/r/126455/


- David


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126474/#review89939
---


On Dec. 22, 2015, 4:52 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126474/
> ---
> 
> (Updated Dec. 22, 2015, 4:52 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> A static QRegExp was used but it is not thread safe. QRegularExpression
> seems to be.
> 
> BUG: 352356
> 
> 
> Diffs
> -
> 
>   src/urifilters/shorturi/kshorturifilter.cpp 
> 1af4768b7b5ab9d1f5af52f17170d466d854b9bb 
> 
> Diff: https://git.reviewboard.kde.org/r/126474/diff/
> 
> 
> Testing
> ---
> 
> I ran the autotests which includes urifilter and I've run krunner which uses 
> it extensively.
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126474: Port QRegExp to QRegularExpression in kshorturifilter

2015-12-22 Thread Ivan Čukić 


> On Dec. 22, 2015, 4:59 p.m., Ivan Čukić wrote:
> > The patch seems straight-forward.
> > 
> > I can not find the reference that QRegularExpression is thread-safe (the 
> > most I see is 'all functions in this class are reentrant which is stated 
> > for QRegExp as well).
> > 
> > The QRegExp vs QRegularExpression situation is quite confusing, but the 
> > later should be preferred in Qt 5.x - it is faster if nothing else.
> 
> David Edmundson wrote:
> you're right docs don't mention it, but see Alex's comment on 
> https://git.reviewboard.kde.org/r/126455/

It wasn't that I don't trust you on its thread safety. :)

Just meant to leave it as a note for future reference if they decide to remove 
the 'inner mutex' during some code clean-up and optimization attempts.


- Ivan


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126474/#review89939
---


On Dec. 22, 2015, 4:52 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126474/
> ---
> 
> (Updated Dec. 22, 2015, 4:52 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> A static QRegExp was used but it is not thread safe. QRegularExpression
> seems to be.
> 
> BUG: 352356
> 
> 
> Diffs
> -
> 
>   src/urifilters/shorturi/kshorturifilter.cpp 
> 1af4768b7b5ab9d1f5af52f17170d466d854b9bb 
> 
> Diff: https://git.reviewboard.kde.org/r/126474/diff/
> 
> 
> Testing
> ---
> 
> I ran the autotests which includes urifilter and I've run krunner which uses 
> it extensively.
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126474: Port QRegExp to QRegularExpression in kshorturifilter

2015-12-28 Thread David Edmundson 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126474/
---

(Updated Dec. 28, 2015, 2:20 p.m.)


Review request for KDE Frameworks.


Changes
---

Added exciting comment


Repository: kio


Description
---

A static QRegExp was used but it is not thread safe. QRegularExpression
seems to be.

BUG: 352356


Diffs (updated)
-

  src/urifilters/shorturi/kshorturifilter.cpp 
6002ec6925c0acdd20a053f98baca46863f69fa6 

Diff: https://git.reviewboard.kde.org/r/126474/diff/


Testing
---

I ran the autotests which includes urifilter and I've run krunner which uses it 
extensively.


Thanks,

David Edmundson

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126474: Port QRegExp to QRegularExpression in kshorturifilter

2015-12-28 Thread Ivan Čukić 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126474/#review90216
---


I'd say 'Ship it' on this, or wait for D. Faure. (iirc, he is the kio 
maintainer)

- Ivan Čukić


On Dec. 28, 2015, 2:20 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126474/
> ---
> 
> (Updated Dec. 28, 2015, 2:20 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> A static QRegExp was used but it is not thread safe. QRegularExpression
> seems to be.
> 
> BUG: 352356
> 
> 
> Diffs
> -
> 
>   src/urifilters/shorturi/kshorturifilter.cpp 
> 6002ec6925c0acdd20a053f98baca46863f69fa6 
> 
> Diff: https://git.reviewboard.kde.org/r/126474/diff/
> 
> 
> Testing
> ---
> 
> I ran the autotests which includes urifilter and I've run krunner which uses 
> it extensively.
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126474: Port QRegExp to QRegularExpression in kshorturifilter

2016-01-01 Thread David Faure 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126474/#review90403
---

Ship it!



src/urifilters/shorturi/kshorturifilter.cpp (line 58)


"despite" sounds like the api docs say that it's not thread safe. AFAICS 
the docs don't say anything either way. I agree that one shouldn't assume 
thread-safety unless explicitly documented, but I think it's just an omission 
in the doc, the whole point of the QRegularExpression API is thread safety.


- David Faure


On Dec. 28, 2015, 2:20 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126474/
> ---
> 
> (Updated Dec. 28, 2015, 2:20 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> A static QRegExp was used but it is not thread safe. QRegularExpression
> seems to be.
> 
> BUG: 352356
> 
> 
> Diffs
> -
> 
>   src/urifilters/shorturi/kshorturifilter.cpp 
> 6002ec6925c0acdd20a053f98baca46863f69fa6 
> 
> Diff: https://git.reviewboard.kde.org/r/126474/diff/
> 
> 
> Testing
> ---
> 
> I ran the autotests which includes urifilter and I've run krunner which uses 
> it extensively.
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126474: Port QRegExp to QRegularExpression in kshorturifilter

2016-01-01 Thread David Faure 


> On Jan. 1, 2016, 5:15 p.m., David Faure wrote:
> > src/urifilters/shorturi/kshorturifilter.cpp, line 58
> > 
> >
> > "despite" sounds like the api docs say that it's not thread safe. 
> > AFAICS the docs don't say anything either way. I agree that one shouldn't 
> > assume thread-safety unless explicitly documented, but I think it's just an 
> > omission in the doc, the whole point of the QRegularExpression API is 
> > thread safety.

I talked to Giuseppe, he'll update the docu to mention thread safety.


- David


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126474/#review90403
---


On Dec. 28, 2015, 2:20 p.m., David Edmundson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126474/
> ---
> 
> (Updated Dec. 28, 2015, 2:20 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> A static QRegExp was used but it is not thread safe. QRegularExpression
> seems to be.
> 
> BUG: 352356
> 
> 
> Diffs
> -
> 
>   src/urifilters/shorturi/kshorturifilter.cpp 
> 6002ec6925c0acdd20a053f98baca46863f69fa6 
> 
> Diff: https://git.reviewboard.kde.org/r/126474/diff/
> 
> 
> Testing
> ---
> 
> I ran the autotests which includes urifilter and I've run krunner which uses 
> it extensively.
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 126474: Port QRegExp to QRegularExpression in kshorturifilter

2016-01-02 Thread David Edmundson 

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/126474/
---

(Updated Jan. 2, 2016, 10:32 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks.


Changes
---

Submitted with commit 6763f7a6cab41fc4c94be783305ac2c8e85ccaa1 by David 
Edmundson to branch master.


Repository: kio


Description
---

A static QRegExp was used but it is not thread safe. QRegularExpression
seems to be.

BUG: 352356


Diffs
-

  src/urifilters/shorturi/kshorturifilter.cpp 
6002ec6925c0acdd20a053f98baca46863f69fa6 

Diff: https://git.reviewboard.kde.org/r/126474/diff/


Testing
---

I ran the autotests which includes urifilter and I've run krunner which uses it 
extensively.


Thanks,

David Edmundson

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel