From kde-core-devel Sat Apr 20 14:11:35 2013 From: "Rolf Eike Beer" Date: Sat, 20 Apr 2013 14:11:35 +0000 To: kde-core-devel Subject: Re: Review Request 110091: clean up and update FreeBSD support for kinfocenter Message-Id: <20130420141135.18348.93100 () vidsolbach ! de> X-MARC-Message: https://marc.info/?l=kde-core-devel&m=136646711716422 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--===============7172037709337372880==" --===============7172037709337372880== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110091/#review31332 ----------------------------------------------------------- cmake/modules/FindLibdevinfo.cmake Why is this necessary at all? If you don't have a strong reason for it just drop it. cmake/modules/FindLibdevinfo.cmake Please read Modules/readme.txt from CMake about how to name such variables. Short: this should be DEVINFO_INCLUDE_DIR, and you should have a DEVINFO_INCLUDE_DIRS later, which is not cached. cmake/modules/FindLibdevinfo.cmake Has the header a version number one could parse out and use? kinfocenter/Modules/base/CMakeLists.txt The TODO comment above can go away now I think. kinfocenter/Modules/base/info_fbsd.cpp Why not just use QProcess here to get the result? I fear this stuff dates back to QT(<=3) times where this probably had issues, but that isn't true anymore. kinfocenter/Modules/base/info_fbsd.cpp QProcess here, too. Otherwise it may be a good idea to just put your patch in now as it is and do a followup patch that just kills that function and replaces it with QProcess, or keeps that functions and gives it a proper QProcess-based interface, i.e. separate arguments passed as a QStringList and such. kinfocenter/Modules/base/info_fbsd.cpp This line is flagged to have inconsistent whitespace. kinfocenter/Modules/info/CMakeLists.txt I miss a include_directories(${DEVINFO_INCLUDE_DIRS}) or something like that here. - Rolf Eike Beer On April 19, 2013, 10:14 p.m., Max Brazhnikov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/110091/ > ----------------------------------------------------------- > > (Updated April 19, 2013, 10:14 p.m.) > > > Review request for kde-workspace. > > > Description > ------- > > Add FindLibdevinfo.cmake > Clean up info_fbsd.cpp and utilize PCIUtils > > > Diffs > ----- > > cmake/modules/FindLibdevinfo.cmake e69de29 > kinfocenter/Modules/base/CMakeLists.txt 2b3c34e > kinfocenter/Modules/base/info_fbsd.cpp 6bbaa1a > kinfocenter/Modules/info/CMakeLists.txt dba6bc7 > > Diff: http://git.reviewboard.kde.org/r/110091/diff/ > > > Testing > ------- > > > Thanks, > > Max Brazhnikov > > --===============7172037709337372880== Content-Type: text/html; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit
This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/110091/

cmake/modules/FindLibdevinfo.cmake (Diff revision 1) 

    8
    
if(DEVINFO_INCLUDES AND DEVINFO_LIBRARIES)

  


  

    
    

    9
    
   set(DEVINFO_FIND_QUIETLY TRUE)

  


  

    
    

    10
    
endif(DEVINFO_INCLUDES AND DEVINFO_LIBRARIES)

  


  

    
    

    11
    

  


 





Why is this necessary at all? If you don't have a strong reason for it just drop it.














 
  

   
    cmake/modules/FindLibdevinfo.cmake
    

     (Diff revision 1)

    
   
  

 



 
 

 

  

    
    

    12
    
find_path(DEVINFO_INCLUDES devinfo.h)

  


 





Please read Modules/readme.txt from CMake about how to name such variables. Short: this should be DEVINFO_INCLUDE_DIR, and you should have a DEVINFO_INCLUDE_DIRS later, which is not cached.














 
  

   
    cmake/modules/FindLibdevinfo.cmake
    

     (Diff revision 1)

    
   
  

 



 
 

 

  

    
    

    16
    
include(FindPackageHandleStandardArgs)

  


 





Has the header a version number one could parse out and use?















 
  

   
    kinfocenter/Modules/base/CMakeLists.txt
    

     (Diff revision 1)

    
   
  

 



 
 

 

  

    
    

    5
    
macro_bool_to_01(DEVINFO_FOUND HAVE_DEVINFO_H)

  


 





The TODO comment above can go away now I think.














 
  

   
    kinfocenter/Modules/base/info_fbsd.cpp
    

     (Diff revision 1)

    
   
  

 



 
 

 

  

    
    

    97
    
	if (GetInfo_ReadfromPipe(tree, "/sbin/camcontrol devlist 2>&1", true))

  


 





Why not just use QProcess here to get the result? I fear this stuff dates back to QT(<=3) times where this probably had issues, but that isn't true anymore.














 
  

   
    kinfocenter/Modules/base/info_fbsd.cpp
    

     (Diff revision 1)

    
   
  

 



 
 

 

  

    142
    

    124
    
	if (GetInfo_ReadfromPipe(tree, "/usr/sbin/pciconf -l -v 2>&1", true))

  


 





QProcess here, too. Otherwise it may be a good idea to just put your patch in now as it is and do a followup patch that just kills that function and replaces it with QProcess, or keeps that functions and gives it a proper QProcess-based interface, i.e. separate arguments passed as a QStringList and such.














 
  

   
    kinfocenter/Modules/base/info_fbsd.cpp
    

     (Diff revision 1)

    
   
  

 

 
  


   
int print_resource(struct devinfo_res *res, void *arg)


  

 



 
 

 

  

    239
    
	s += tmp;

    190
    
 		result << showbase << hex;

  


 





This line is flagged to have inconsistent whitespace.














 
  

   
    kinfocenter/Modules/info/CMakeLists.txt
    

     (Diff revision 1)

    
   
  

 



 
 

 

  

    15
    
target_link_libraries(kcm_info  ${KDE4_KDEUI_LIBS} ${QT_QTGUI_LIBRARY} ${X11_X11_LIB})

    15
    
target_link_libraries(kcm_info  ${KDE4_KDEUI_LIBS} ${QT_QTGUI_LIBRARY} ${X11_X11_LIB})

  


 





I miss a include_directories(${DEVINFO_INCLUDE_DIRS}) or something like that here.









- Rolf Eike






On April 19th, 2013, 10:14 p.m. UTC, Max Brazhnikov wrote:











 

  


Review request for kde-workspace.


By Max Brazhnikov.




Updated April 19, 2013, 10:14 p.m.








Description 

 

 

  
   
Add FindLibdevinfo.cmake
Clean up info_fbsd.cpp and utilize PCIUtils


  		
 









Diffs 


    

 
  • cmake/modules/FindLibdevinfo.cmake (e69de29)
    • 

 
  • kinfocenter/Modules/base/CMakeLists.txt (2b3c34e)
    • 

 
  • kinfocenter/Modules/base/info_fbsd.cpp (6bbaa1a)
    • 

 
  • kinfocenter/Modules/info/CMakeLists.txt (dba6bc7)
    • 





View Diff








  		
 











  

 



--===============7172037709337372880==--