Filter out warnings as error from parser's compile flags
ClosedPublic

Authored by gracicot on Sep 25 2017, 5:32 AM.

Details

Summary

The highlighting in my project was all screwed up like in the kdevelop codebase last week.

Now kdevelop seems fine parsing it's own code, but my codebase highlighting was still screwed up. Removing warnings as error in the parser fixed the problem.

Since warnings as error can be enabled with -Werror for all of them, and individually with -Werror=, I thought disabling both. So a list of argument like that:

$ clang++ -Wall -Werror -Werror=unreachable-code

Becomes:

$ clang++ -Wall -Wunreachable-code

Now the parser is no longer reporting warnings as error, but still reporting unreachable code.

Diff Detail

Repository
R32 KDevelop
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
gracicot created this revision.Sep 25 2017, 5:32 AM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptSep 25 2017, 5:32 AM
gracicot edited the summary of this revision. (Show Details)Sep 25 2017, 5:33 AM
gracicot added a project: KDevelop.
kfunk added a comment.Sep 25 2017, 8:15 AM

I wonder if this is being done at the wrong layer. This should likely be done for every build system. We could probably strip these flags in the custom-definesandincludes plugin, which is using the data from the CMake plugin and other build system plugins internally.

Anyhow: What was the problem in KDevelop for you to begin with? Got a screenshot?

brauch added a subscriber: brauch.Sep 25 2017, 9:17 AM

I was experimenting with a similar patch locally and yes, I think this should be done on the higher layer as well. Otherwise, you need the same patch for e.g. qmake.

mwolff added a subscriber: mwolff.Sep 25 2017, 9:38 AM

Imo this should be done in the clang plugin. Other plugins may want to know the "full" args, so filtering in the IADM is too early. Only for the purpose of clang do we want to filter this out.

kfunk added a comment.Sep 25 2017, 9:41 AM

Right, agreed.

@gracicot Want to look into it? Relevant files should be plugins/clang/duchain/parsesession.cpp.

Imo this should be done in the clang plugin. Other plugins may want to know the "full" args, so filtering in the IADM is too early. Only for the purpose of clang do we want to filter this out.

Right, I agree as well. Didn't think about that.

@kfunk I'll check this out soon!

gracicot updated this revision to Diff 19922.Sep 26 2017, 1:38 AM
  • Revert "Filter out warnings as error from parser's compile flags"
  • Filter clang flags in the clang parser
mwolff requested changes to this revision.Sep 26 2017, 8:00 AM
mwolff added inline comments.
plugins/clang/duchain/parsesession.cpp
68

move { to its own line

71

this should use QVector::erase + std::remove_if on a mutable container

This revision now requires changes to proceed.Sep 26 2017, 8:00 AM

Otherwise, looks good to me, thanks a lot!

plugins/clang/duchain/parsesession.cpp
114

not sure what std::move accomplishes here?

mwolff added inline comments.Sep 26 2017, 8:30 AM
plugins/clang/duchain/parsesession.cpp
114

it's just overly complicated, imo. a plain remove_if + erase would achieve the same, without moving and without constructing a second container, thus probably being faster too (and less code!)

gracicot added inline comments.Sep 26 2017, 1:33 PM
plugins/clang/duchain/parsesession.cpp
114

I was focused on returning a new, filtered container, instead of just changing the current one. Sorry, I'll fix it soon. Also, I just realized I take the container by const ref, so this std::move is completely ineffective indeed.

gracicot updated this revision to Diff 19957.Sep 26 2017, 9:54 PM
  • Changed std::copy_if to erase + remove_if in the same container
brauch accepted this revision.Sep 26 2017, 10:00 PM

Looks good to me, thanks! Maybe wait for Milian to approve as well.

mwolff requested changes to this revision.Sep 26 2017, 11:33 PM

minor nits, lgtm in general now

plugins/clang/duchain/parsesession.cpp
68

please add a comment explaining why this is needed (it's clear now, but for the future it would be helpful to have a short reminder). Also note down why you handle both, -Werror and -Werror= (give an example for the latter). It took me a bit to remember what that is for

71

join next line

73

const&

78

only on start, no? I.e. we don't want to replace that somewhere in the middle. So instead do a startsWith check and then replace the start range with "-W"

This revision now requires changes to proceed.Sep 26 2017, 11:33 PM
brauch added inline comments.Sep 26 2017, 11:35 PM
plugins/clang/duchain/parsesession.cpp
73

this can't be const, it's modified

gracicot edited the summary of this revision. (Show Details)Sep 27 2017, 2:47 AM
gracicot updated this revision to Diff 19964.Sep 27 2017, 2:54 AM
gracicot edited the summary of this revision. (Show Details)
  • Remove '-Werror=' properly and only if found at the start of the string
mwolff requested changes to this revision.Sep 27 2017, 7:14 AM

more nits, sorry :P

plugins/clang/duchain/parsesession.cpp
68

please add a comment here as I requested the last time

72

hmmm I like the brevity of this code, I'm not too sure what others think. mutating from within a remove_if callback? Haven't seen this before, but it should work so let's keep it this way. please do add a comment here though.

77

use QByteArrayLiteral:

const auto asError = QByteArrayLiteral("-Werror=");
79

remove empty line

This revision now requires changes to proceed.Sep 27 2017, 7:14 AM
mwolff added inline comments.Sep 27 2017, 7:28 AM
plugins/clang/duchain/parsesession.cpp
72

discussing this with a colleague, he also agrees that this is quite uncommon. Could you please separate the code using two algorithms for readability? Sure, it will be marginally slower than, but easier to grasp for readers and also more failsafe. i.e. split it up like so:

// step one: remove -Werror
erase + remove_if
// step two: replace -Werror= with -W
transform or for_each

Actually, thinking out loud - could you simply always only do a transform/foreach and replace the -Werror args with empty strings?

Also, I'd love to see this tested, but it's probably going to be somewhat complicated to do that. TestDUChain::testGccCompatibility could be used as a basis, i.e. to setup a custom project with some custom compiler flags that trigger the test for both to check that we only report warnings, not errors

gracicot updated this revision to Diff 20223.Oct 2 2017, 4:56 AM
  • Clear -Werror instead of removing it
mwolff requested changes to this revision.Oct 4 2017, 4:26 PM

I'll clean this up and commit it to 5.2, thanks!

plugins/clang/duchain/parsesession.cpp
76

move out of the loop

77

startsWith

79

add a comment:

// replace -Werror=foo with -Wfoo
This revision now requires changes to proceed.Oct 4 2017, 4:26 PM
mwolff added a comment.Oct 4 2017, 4:27 PM

Actually, I'll need to know your email address - otherwise I cannot commit it for you.

Thanks

gracicot added inline comments.Oct 4 2017, 7:22 PM
plugins/clang/duchain/parsesession.cpp
76

Well... I don't really need it outside the loop why should I put it in a wider scope?

As I said, I also need your email address so I (or someone else) can push this for you

thanks

plugins/clang/duchain/parsesession.cpp
76

because it's constant and doesn't depend on the loop arguments. It's always a good idea to hoist stuff out of the loop in such situations. Maybe the compiler is smart enough, but doing it manually doesn't hurt and ensures it's always hoisted out.

gracicot updated this revision to Diff 20458.Oct 8 2017, 12:14 AM
  • Added a comment to explain the operations done to warning string and optimizations
gracicot updated this revision to Diff 20459.Oct 8 2017, 12:18 AM
  • Removed else brakets in else if

My email is gufideg at gmail.com. Thank you for your patience in this review.

This revision was automatically updated to reflect the committed changes.