Fix CompilerFilterStrategy::errorInLine() to handle paths with whitespace
ClosedPublic

Authored by kossebau on May 28 2017, 1:27 AM.

Details

Summary

The "([^: \\t]+):" does not match strings with white space. While this
reduces the chance of wrong matches, it also excludes valid ones for
paths with white space in them (like on new KDE CI).

The different regex for the different tools are a bit inconsistent on that
matter, so as consistent solution whitespaces in paths are supported.

Tests are adapted to test with both paths with spaces and paths without
(for now only unix-style, given the patterns currently used).

Test Plan

test_filteringstrategy no longer fails when sources are below a path
with white spaces. And cmake/gcc errors in projects with such paths
are now also properly supported.

Diff Detail

Repository
R33 KDevPlatform
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kossebau created this revision.May 28 2017, 1:27 AM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptMay 28 2017, 1:27 AM

^([^:\\t]+): with my little regexp knowledge confuses me a little. Why is the : also in the [^]? Was that needed for a non-greedy match, at least with the old QRegExp?

Ah, and what about Windows-like path? Only expect this with MSVC?

Ideally the tests would be updated to cover a range of such paths, but not sure why projectPath() from testlinebuilderfunctions.h uses __FILE__ + non-existing dir instead of some conpletely hardcoded non-existant path?

volden edited edge metadata.May 28 2017, 12:17 PM

Ah, and what about Windows-like path? Only expect this with MSVC?

Ideally the tests would be updated to cover a range of such paths, but not sure why projectPath() from testlinebuilderfunctions.h uses __FILE__ + non-existing dir instead of some conpletely hardcoded non-existant path?

The projectPath function was introduced to test the regexes on as many combinations of paths as possible. Truth be told, I'm not as fond of it today as when I originally wrote it. Feel free to change that into testing fixed paths explicitly (With and without space )
:-)

^([^:\\t]+): with my little regexp knowledge confuses me a little. Why is the : also in the [^]? Was that needed for a non-greedy match, at least with the old QRegExp?

I want to say they are there for a good reason, but I honestly don't remember it. Maybe they were there from the beginning? IIRC the regexes were originally inspired by some in EMACS.

apol requested changes to this revision.May 28 2017, 8:40 PM
apol added a subscriber: apol.

If this is making sure some output match that didn't use to, I suggest adding a test case so that we don't regress in changes like this in the future.

This revision now requires changes to proceed.May 28 2017, 8:40 PM
kossebau updated this revision to Diff 14921.May 29 2017, 1:45 AM
kossebau edited edge metadata.

change unit tests to test against hardcoded paths,
both without whitespace and with spaces
(windows variants left for somebody on windows)

kossebau edited the summary of this revision. (Show Details)May 29 2017, 1:48 AM
kfunk accepted this revision.May 29 2017, 6:50 AM

@apol: okay with this? phab insists you give your blessing as well, given your previous auto-non-accepting-tagged comment :)

apol accepted this revision.May 29 2017, 9:33 AM
This revision is now accepted and ready to land.May 29 2017, 9:33 AM
This revision was automatically updated to reflect the committed changes.