D23801: Port kpac from QtScript

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> carewolf wrote in script.cpp:316
> Sure, as I said, I only write it this way because a copy should be taken, and 
> while Qt can work around declaring an async argument as a reference, I still 
> consider it bad style to make that mistake.
> 
> In any case the difference is basically academic when it comes reference 
> counted Qt containers. You can save some nanoseconds on doing a reference 
> pass when it isn't async, but I have wasted more time hunting down obscure 
> bugs caused by using references in cross-thread methods in other frameworks, 
> so I prefer this. Feel free to change it if you like though. As I said it is 
> just a best practice/coding style for me, and not necessary for Qt invokables.

Okay, think I understood. Thanks for your answers :)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D23801

To: carewolf, dfaure
Cc: kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23801: Port kpac from QtScript

[Allan Sandfeld Jensen]

carewolf added inline comments.

INLINE COMMENTS

> kossebau wrote in script.cpp:316
> With queued signals, any const-reference arguments are passed via an internal 
> value-copy IIRC, so references are not out-dated.
> Can the same technique not be expected with any usages of invocables, like 
> from scripting engines?

Sure, as I said, I only write it this way because a copy should be taken, and 
while Qt can work around declaring an async argument as a reference, I still 
consider it bad style to make that mistake.

In any case the difference is basically academic when it comes reference 
counted Qt containers. You can save some nanoseconds on doing a reference pass 
when it isn't async, but I have wasted more time hunting down obscure bugs 
caused by using references in cross-thread methods in other frameworks, so I 
prefer this. Feel free to change it if you like though. As I said it is just a 
best practice/coding style for me, and not necessary for Qt invokables.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D23801

To: carewolf, dfaure
Cc: kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23801: Port kpac from QtScript

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> carewolf wrote in script.cpp:316
> Invokables can be called asynchronously (queued), if you take a reference and 
> the isn't evaluated immediatly the reference might be invalid by the time the 
> call is evaluated.

With queued signals, any const-reference arguments are passed via an internal 
value-copy IIRC, so references are not out-dated.
Can the same technique not be expected with any usages of invocables, like from 
scripting engines?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D23801

To: carewolf, dfaure
Cc: kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23801: Port kpac from QtScript

[Allan Sandfeld Jensen]

carewolf added inline comments.

INLINE COMMENTS

> kossebau wrote in script.cpp:316
> So why would it be preferable to have the value be copied there? In general, 
> one prefers to avoid copies, so what is the different motvation here?

Invokables can be called asynchronously (queued), if you take a reference and 
the isn't evaluated immediatly the reference might be invalid by the time the 
call is evaluated.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D23801

To: carewolf, dfaure
Cc: kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23801: Port kpac from QtScript

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> carewolf wrote in script.cpp:316
> Not really. I prefer to do it that way for invokable as the value will be 
> copied, but const ref works too.

So why would it be preferable to have the value be copied there? In general, 
one prefers to avoid copies, so what is the different motvation here?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D23801

To: carewolf, dfaure
Cc: kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23801: Port kpac from QtScript

[Allan Sandfeld Jensen]

carewolf added inline comments.

INLINE COMMENTS

> kossebau wrote in script.cpp:316
> Hi @carewolf . Curious: Why all the non-const-ref QString argument types here?
> 
> Is there a need to do that with Q_INVOKABLE methods somehow?
> Asking because clazy falgs these, compare D25039 
> 

Not really. I prefer to do it that way for invokable as the value will be 
copied, but const ref works too.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D23801

To: carewolf, dfaure
Cc: kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23801: Port kpac from QtScript

[Friedrich W. H. Kossebau]

kossebau added inline comments.

INLINE COMMENTS

> script.cpp:316
>  // @returns true if @p str matches the shell @p pattern
> -QScriptValue ShExpMatch(QScriptContext *context, QScriptEngine *engine)
> +Q_INVOKABLE QJSValue ShExpMatch(QString str, QString patternStr)
>  {

Hi @carewolf . Curious: Why all the non-const-ref QString argument types here?

Is there a need to do that with Q_INVOKABLE methods somehow?
Asking because clazy falgs these, compare D25039 


REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D23801

To: carewolf, dfaure
Cc: kossebau, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23801: Port kpac from QtScript

[Allan Sandfeld Jensen]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:4ad1902278f0: Port kpac from QtScript (authored by 
carewolf).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23801?vs=65690&id=65741

REVISION DETAIL
  https://phabricator.kde.org/D23801

AFFECTED FILES
  src/kpac/CMakeLists.txt
  src/kpac/script.cpp
  src/kpac/script.h

To: carewolf, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23801: Port kpac from QtScript

[David Faure]

dfaure accepted this revision.
dfaure added a comment.
This revision is now accepted and ready to land.


  I'll trust your expertise with such ports.
  I wouldn't know how to test this either.

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D23801

To: carewolf, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23801: Port kpac from QtScript

[Sandro Knauß]

knauss added a task: T11530: Investigate removal of KIO's KPAC in favor of 
QNetworkProxy.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D23801

To: carewolf, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D23801: Port kpac from QtScript

[Allan Sandfeld Jensen]

carewolf created this revision.
carewolf added a reviewer: dfaure.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
carewolf requested review of this revision.

REVISION SUMMARY
  Migrate the QtScript code to QJS classes.
  
  Warning: untested

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D23801

AFFECTED FILES
  src/kpac/CMakeLists.txt
  src/kpac/script.cpp
  src/kpac/script.h

To: carewolf, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns