<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/115014/">https://git.reviewboard.kde.org/r/115014/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 14th, 2014, 4:08 p.m. UTC, <b>Kevin Funk</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">By the way, the filteringstrateytest needs a major overhaul. Not having the test input inline in the test file is very confusing (cf. testlinebuilderfunctions.h)
Other functions, such as FilteringStrategyTest::testCompilerFilterstrategy don't really check if we've been able to parse the url (and line/col) information. There's FilteringStrategyTest::testExtractionOfLineAndCulmn, which handles that, but only for a subset of the possible cases.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I don't think that there was any deeper thought behind the requirement of both line number and URL. IIRC all the filters where it made sense to make the line clickable had both the url and line number at the time - feel free to change that.
The original idea was that tests are divided so that testXXFilterstrategy-tests only tests regular expressions for a given filter behaves as intended. The test for extraction of line and URL should cover only that.
Looking at it again, I think I agree that it makes more sense to tests extraction of line and URL in the testXXFilterstrategy-tests.
IMHO the splitup of the line builder functions and the test makes good sense. The name of the line builder function usually says what it does, and if I need to study it in more detail, I can hover over it and with one click I'm at the function. I usually find it a pain to navigate in files > 1000 LOC. I'm not overly religious about it though :-)
</pre>
<br />
<p>- Morten</p>
<br />
<p>On January 14th, 2014, 4:04 p.m. UTC, Kevin Funk wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDevelop and Morten Volden.</div>
<div>By Kevin Funk.</div>
<p style="color: grey;"><i>Updated Jan. 14, 2014, 4:04 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdevplatform
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Make it possible to click on cmake-automoc error
There's a change needed in outputfilteringstrategies to be able to click
on error items only containing an url (no line information).
Morten, any reason you try to enforce having a line number?
It's still useful, even if we just have the url, imo.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>outputview/outputfilteringstrategies.cpp <span style="color: grey">(0dcabd8614dfc160fad612a077150ec297eb3735)</span></li>
<li>outputview/tests/filteringstrategytest.cpp <span style="color: grey">(1177c9aaa32361de5164404639e8d855e53019e3)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/115014/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>