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