Re: Review Request 122493: Use math.h round rather than C++11 std::lround.

2015-02-09 Thread Marco Martin


 On Feb. 9, 2015, 12:59 a.m., Mark Gaiser wrote:
  -1
  
  You use C-Style casts. Oke, the frameworks coding style doesn't seem to 
  explicitly forbid it (casts are not mentioned), but if i recall correctly 
  we use the Qt style + some of our own which means we should obey the Qt 
  style [1] which does mention casts and forbids the C-Sytle cast.
  
  Secondly, you seem to be trying to get this working on a compiler that 
  isn't supported [2]. If one of those compilers have issues with std::lround 
  (which i seriously doubt) then we should perhaps look into replacing it 
  with qRound. However, Qt is also moving away from it's own algorithms in 
  favor of the stl ones so we should stick to the std:: versions.
  
  Cheers
  
  [1] http://qt-project.org/wiki/Qt_Coding_Style
  [2] 
  https://community.kde.org/Frameworks/Policies#Frameworks_compiler_requirements_and_C.2B.2B11
 
 Ivan Čukić wrote:
 I'm torn on this one.
 
 This is a minor edit (mainly thinking about the potential qRound version) 
 that would fix the current master to work with an old platform. Without 
 having any downsides (apart from possible future depreciacion of qRound).
 
 But, on the other hand, something else will probably be brought in soon 
 that will require some more essential c++11 things, and break on osx 10.7. 
 So, one wonders whether a patch that just prolongs the inevitable has a 
 purpose. :)
 
 Kevin Funk wrote:
 Not sure if it's worth discussion this issue a lot... `qRound` isn't 
 deprecated yet, used a lot throughout frameworks, and it doesn't look like it 
 will be deprecated (I've yet to see a discussion on the Qt ML about this). 
 Yet `qRound` is way more pleasant to read than `static_castint(...)`.

ok for using qRound for now if it fixes build on old platforms


- Marco


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


On Feb. 8, 2015, 11:12 p.m., Jeremy Whiting wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122493/
 ---
 
 (Updated Feb. 8, 2015, 11:12 p.m.)
 
 
 Review request for KDE Frameworks and Marco Martin.
 
 
 Repository: kdeclarative
 
 
 Description
 ---
 
 Use math.h round rather than C++11 std::lround.
 
 Removes dependency on C++11.
 
 
 Diffs
 -
 
   src/qmlcontrols/kquickcontrolsaddons/plotter.cpp 
 67ce63a943234b167165b0f3986f974bba5ff0cf 
 
 Diff: https://git.reviewboard.kde.org/r/122493/diff/
 
 
 Testing
 ---
 
 kdeclarative is able to build on OS X 10.7 with the built in XCode compiler 
 and standard library.
 
 
 Thanks,
 
 Jeremy Whiting
 


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


Re: Review Request 122493: Use math.h round rather than C++11 std::lround.

2015-02-09 Thread Ivan Čukić


 On Feb. 9, 2015, 12:59 a.m., Mark Gaiser wrote:
  -1
  
  You use C-Style casts. Oke, the frameworks coding style doesn't seem to 
  explicitly forbid it (casts are not mentioned), but if i recall correctly 
  we use the Qt style + some of our own which means we should obey the Qt 
  style [1] which does mention casts and forbids the C-Sytle cast.
  
  Secondly, you seem to be trying to get this working on a compiler that 
  isn't supported [2]. If one of those compilers have issues with std::lround 
  (which i seriously doubt) then we should perhaps look into replacing it 
  with qRound. However, Qt is also moving away from it's own algorithms in 
  favor of the stl ones so we should stick to the std:: versions.
  
  Cheers
  
  [1] http://qt-project.org/wiki/Qt_Coding_Style
  [2] 
  https://community.kde.org/Frameworks/Policies#Frameworks_compiler_requirements_and_C.2B.2B11

I'm torn on this one.

This is a minor edit (mainly thinking about the potential qRound version) that 
would fix the current master to work with an old platform. Without having any 
downsides (apart from possible future depreciacion of qRound).

But, on the other hand, something else will probably be brought in soon that 
will require some more essential c++11 things, and break on osx 10.7. So, one 
wonders whether a patch that just prolongs the inevitable has a purpose. :)


- Ivan


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


On Feb. 8, 2015, 11:12 p.m., Jeremy Whiting wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122493/
 ---
 
 (Updated Feb. 8, 2015, 11:12 p.m.)
 
 
 Review request for KDE Frameworks and Marco Martin.
 
 
 Repository: kdeclarative
 
 
 Description
 ---
 
 Use math.h round rather than C++11 std::lround.
 
 Removes dependency on C++11.
 
 
 Diffs
 -
 
   src/qmlcontrols/kquickcontrolsaddons/plotter.cpp 
 67ce63a943234b167165b0f3986f974bba5ff0cf 
 
 Diff: https://git.reviewboard.kde.org/r/122493/diff/
 
 
 Testing
 ---
 
 kdeclarative is able to build on OS X 10.7 with the built in XCode compiler 
 and standard library.
 
 
 Thanks,
 
 Jeremy Whiting
 


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


Re: Review Request 122493: Use math.h round rather than C++11 std::lround.

2015-02-09 Thread Kevin Funk


 On Feb. 9, 2015, 12:59 a.m., Mark Gaiser wrote:
  -1
  
  You use C-Style casts. Oke, the frameworks coding style doesn't seem to 
  explicitly forbid it (casts are not mentioned), but if i recall correctly 
  we use the Qt style + some of our own which means we should obey the Qt 
  style [1] which does mention casts and forbids the C-Sytle cast.
  
  Secondly, you seem to be trying to get this working on a compiler that 
  isn't supported [2]. If one of those compilers have issues with std::lround 
  (which i seriously doubt) then we should perhaps look into replacing it 
  with qRound. However, Qt is also moving away from it's own algorithms in 
  favor of the stl ones so we should stick to the std:: versions.
  
  Cheers
  
  [1] http://qt-project.org/wiki/Qt_Coding_Style
  [2] 
  https://community.kde.org/Frameworks/Policies#Frameworks_compiler_requirements_and_C.2B.2B11
 
 Ivan Čukić wrote:
 I'm torn on this one.
 
 This is a minor edit (mainly thinking about the potential qRound version) 
 that would fix the current master to work with an old platform. Without 
 having any downsides (apart from possible future depreciacion of qRound).
 
 But, on the other hand, something else will probably be brought in soon 
 that will require some more essential c++11 things, and break on osx 10.7. 
 So, one wonders whether a patch that just prolongs the inevitable has a 
 purpose. :)

Not sure if it's worth discussion this issue a lot... `qRound` isn't deprecated 
yet, used a lot throughout frameworks, and it doesn't look like it will be 
deprecated (I've yet to see a discussion on the Qt ML about this). Yet `qRound` 
is way more pleasant to read than `static_castint(...)`.


- Kevin


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


On Feb. 8, 2015, 11:12 p.m., Jeremy Whiting wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122493/
 ---
 
 (Updated Feb. 8, 2015, 11:12 p.m.)
 
 
 Review request for KDE Frameworks and Marco Martin.
 
 
 Repository: kdeclarative
 
 
 Description
 ---
 
 Use math.h round rather than C++11 std::lround.
 
 Removes dependency on C++11.
 
 
 Diffs
 -
 
   src/qmlcontrols/kquickcontrolsaddons/plotter.cpp 
 67ce63a943234b167165b0f3986f974bba5ff0cf 
 
 Diff: https://git.reviewboard.kde.org/r/122493/diff/
 
 
 Testing
 ---
 
 kdeclarative is able to build on OS X 10.7 with the built in XCode compiler 
 and standard library.
 
 
 Thanks,
 
 Jeremy Whiting
 


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


Review Request 122493: Use math.h round rather than C++11 std::lround.

2015-02-08 Thread Jeremy Whiting

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

Review request for KDE Frameworks and Marco Martin.


Repository: kdeclarative


Description
---

Use math.h round rather than C++11 std::lround.

Removes dependency on C++11.


Diffs
-

  src/qmlcontrols/kquickcontrolsaddons/plotter.cpp 
67ce63a943234b167165b0f3986f974bba5ff0cf 

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


Testing
---

kdeclarative is able to build on OS X 10.7 with the built in XCode compiler and 
standard library.


Thanks,

Jeremy Whiting

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


Re: Review Request 122493: Use math.h round rather than C++11 std::lround.

2015-02-08 Thread Mark Gaiser

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


-1

You use C-Style casts. Oke, the frameworks coding style doesn't seem to 
explicitly forbid it (casts are not mentioned), but if i recall correctly we 
use the Qt style + some of our own which means we should obey the Qt style [1] 
which does mention casts and forbids the C-Sytle cast.

Secondly, you seem to be trying to get this working on a compiler that isn't 
supported [2]. If one of those compilers have issues with std::lround (which i 
seriously doubt) then we should perhaps look into replacing it with qRound. 
However, Qt is also moving away from it's own algorithms in favor of the stl 
ones so we should stick to the std:: versions.

Cheers

[1] http://qt-project.org/wiki/Qt_Coding_Style
[2] 
https://community.kde.org/Frameworks/Policies#Frameworks_compiler_requirements_and_C.2B.2B11

- Mark Gaiser


On Feb. 8, 2015, 11:12 p.m., Jeremy Whiting wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122493/
 ---
 
 (Updated Feb. 8, 2015, 11:12 p.m.)
 
 
 Review request for KDE Frameworks and Marco Martin.
 
 
 Repository: kdeclarative
 
 
 Description
 ---
 
 Use math.h round rather than C++11 std::lround.
 
 Removes dependency on C++11.
 
 
 Diffs
 -
 
   src/qmlcontrols/kquickcontrolsaddons/plotter.cpp 
 67ce63a943234b167165b0f3986f974bba5ff0cf 
 
 Diff: https://git.reviewboard.kde.org/r/122493/diff/
 
 
 Testing
 ---
 
 kdeclarative is able to build on OS X 10.7 with the built in XCode compiler 
 and standard library.
 
 
 Thanks,
 
 Jeremy Whiting
 


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


Re: Review Request 122493: Use math.h round rather than C++11 std::lround.

2015-02-08 Thread Kevin Funk

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


`qRound`?

- Kevin Funk


On Feb. 8, 2015, 11:12 p.m., Jeremy Whiting wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://git.reviewboard.kde.org/r/122493/
 ---
 
 (Updated Feb. 8, 2015, 11:12 p.m.)
 
 
 Review request for KDE Frameworks and Marco Martin.
 
 
 Repository: kdeclarative
 
 
 Description
 ---
 
 Use math.h round rather than C++11 std::lround.
 
 Removes dependency on C++11.
 
 
 Diffs
 -
 
   src/qmlcontrols/kquickcontrolsaddons/plotter.cpp 
 67ce63a943234b167165b0f3986f974bba5ff0cf 
 
 Diff: https://git.reviewboard.kde.org/r/122493/diff/
 
 
 Testing
 ---
 
 kdeclarative is able to build on OS X 10.7 with the built in XCode compiler 
 and standard library.
 
 
 Thanks,
 
 Jeremy Whiting
 


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