Review Request: Share code between makebuilder and external scripts

Morten Volden mvolden2 at gmail.com
Tue May 8 19:45:44 UTC 2012



> On May 3, 2012, 10:05 a.m., Milian Wolff wrote:
> > outputview/ifilterstrategy.h, line 45
> > <http://git.reviewboard.kde.org/r/104814/diff/1/?file=61753#file61753line45>
> >
> >     I severly dislike this api. please change it to something like
> >     
> >     virtual FilteredItem errorInLine(const QString& line) const = 0;
> >     
> >     then, adapt the FilteredItem to have a
> >     
> >     bool isValid() const;
> >     
> >     furthermore, make it so that a default-constructed FilteredItem() is not seen as valid.
> 
> Morten Volden wrote:
>     If I understand you correctly. You would like this (in outputmodel.cpp):
>     
>             FilteredItem item( line );
>     
>             bool matched = m_filter->isErrorInLine(line, item);
>             if( !matched )
>             {
>                 matched = m_filter->isActionInLine(line, item);
>             }
>     
>     
>     to be replaced by this:
>     
>             FilteredItem item = m_filter->errorInLine(line);
>             if( !item.isValid ) {
>                 item = m_filter->actionInLine(line);
>             }
>     
>     
>     But wouldn't returning a Filtered item (for both error and action) force a construction of such an object twice (instead of once) pr. input line?
> 
> Milian Wolff wrote:
>     yes but this is a cheap struct, don't overoptimize at the cost of readability please
>     
>     also, it should be .isValid() - i.e. call a function here that should decide whether it's valid or not (this should not require an additional data member imo).

IMHO the first is just as readable as the other. However, I realize that I am entering a discussion about aesthetics - which is probably pointless. How about I make some unittest(s) where we can do some measuring. If the first is not significantly faster than the other I'd be happy to change it to what you suggested. 


- Morten


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104814/#review13328
-----------------------------------------------------------


On May 6, 2012, 8:59 p.m., Morten Volden wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104814/
> -----------------------------------------------------------
> 
> (Updated May 6, 2012, 8:59 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Description
> -------
> 
> So far I have:
>    * Moved some of the filtering code
>        - More specifically some of the code for filtering (Compiler output, script output ) 
> 
>    * Extended the outputmodel 
>          - It now inherits from QAbstractListModel and IOutputview model
>          - It is now possible to choose a filtering strategy on the outputview outputmodel
>    * Plus a few other things to make things work
> 
>    * Started doing some Unit test of the different filters
> 
> The solution currently has four filtering strategies:
> 
> * No filter
> * Compiler filter 
> * Script Error flter 
> * StaticAnalysis Filter (for cpp check, krazy, etc)
> 
> I think I have now come to a point where it would be good to get some feedback on the proposed solution
> 
> Work identified that remains to be done (Input is welcome):
> 
> * Finish tests for filter strategies
> * Move MakeoutputModel tests to outputview and extend them
> * Make Makebuildder use the code in outputview. 
> * Make NofilteringStrategy faster (i.e. do not create a filtered item for each line)
> * Make custom buildsystem use the code in outputview - Done (But Needs to be reviewed)
> * Extend filterstrategies with other filters?? (Input on that one is most welcome)
> * Discuss how the filters should be selected via the GUI (and in which use-cases it makes sense to do so)
> 
> 
> Diffs
> -----
> 
>   outputview/CMakeLists.txt 5d9e539 
>   outputview/delegateholder.h PRE-CREATION 
>   outputview/delegateholder.cpp PRE-CREATION 
>   outputview/filtereditem.h PRE-CREATION 
>   outputview/filtereditem.cpp PRE-CREATION 
>   outputview/ifilterstrategy.h PRE-CREATION 
>   outputview/ifilterstrategy.cpp PRE-CREATION 
>   outputview/outputdelegate.h PRE-CREATION 
>   outputview/outputdelegate.cpp PRE-CREATION 
>   outputview/outputfilteringstrategies.h PRE-CREATION 
>   outputview/outputfilteringstrategies.cpp PRE-CREATION 
>   outputview/outputformats.h PRE-CREATION 
>   outputview/outputformats.cpp PRE-CREATION 
>   outputview/outputjob.h c17b592 
>   outputview/outputjob.cpp 5b9db06 
>   outputview/outputmodel.h b4c9631 
>   outputview/outputmodel.cpp 317e3ee 
>   outputview/tests/CMakeLists.txt PRE-CREATION 
>   outputview/tests/filteringstrategytest.h PRE-CREATION 
>   outputview/tests/filteringstrategytest.cpp PRE-CREATION 
>   outputview/tests/outputviewtest.h PRE-CREATION 
>   outputview/tests/outputviewtest.cpp PRE-CREATION 
>   outputview/tests/projects/onefileproject/main.cpp PRE-CREATION 
>   plugins/execute/nativeappjob.cpp 3eb654c 
>   plugins/executescript/executescriptoutputmodel.h 180adbd 
>   plugins/executescript/executescriptoutputmodel.cpp 1c852e9 
>   plugins/executescript/executescriptplugin.h 4eea6a4 
>   plugins/executescript/executescriptplugin.cpp e55b514 
>   plugins/executescript/scriptappconfig.h 2e401ea 
>   plugins/executescript/scriptappconfig.cpp d7ff984 
>   plugins/executescript/scriptappjob.h 48400aa 
>   plugins/executescript/scriptappjob.cpp 3b68ca5 
>   plugins/externalscript/externalscriptjob.h 1922469 
>   plugins/externalscript/externalscriptjob.cpp aeb5a34 
>   plugins/externalscript/externalscriptoutputmodel.cpp 7a3d5d4 
>   plugins/externalscript/externalscriptplugin.h c87aaa8 
>   plugins/externalscript/externalscriptplugin.cpp 6d62b63 
>   vcs/dvcs/dvcsjob.cpp 1e0f187 
> 
> Diff: http://git.reviewboard.kde.org/r/104814/diff/
> 
> 
> Testing
> -------
> 
> Started working on Unittest for filtering strategies. Plan to move most tests from Makeoutputmodel at some point. Plus used the patch for a week now and nothing seemingly breaks...
> 
> 
> Screenshots
> -----------
> 
> CPPCheckBefore
>   http://git.reviewboard.kde.org/r/104814/s/550/
> CppcheckAfter
>   http://git.reviewboard.kde.org/r/104814/s/551/
> QuickCompileBefore
>   http://git.reviewboard.kde.org/r/104814/s/552/
> QuickCompileAfter
>   http://git.reviewboard.kde.org/r/104814/s/553/
> 
> 
> Thanks,
> 
> Morten Volden
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20120508/cf57283a/attachment.html>


More information about the KDevelop-devel mailing list