Alexey Serbin has posted comments on this change.
Change subject: [build-support] added IWYU filter script
Patch Set 2:
Thank you for the review! I'll post a new version as soon as I shorten the
muted list enough as I'm working on https://gerrit.cloudera.org/#/c/4738/. I
think I'll mute the most of the tests and address the 'core' code.
That's to avoid needless rebases.
PS2, Line 19: include-what-you-use
> "include-what-you-use (IWYU) tool"
PS2, Line 20: for
PS2, Line 21: amended
> Is "amended" the right word here? The pragmas silences certain IWYU recomme
In the end -- yes, it silences those. It seems I was trying to say something
else here. But let's put it simple.
PS2, Line 24: # Also, it's possible to address the boost-related headers using
: # mapping (the mapping needs to be implemented).
> Can you provide an example of what this is referring to?
It's already done, actually. I'll remove this comment.
But for the reference, there is a mechanism of so called 'mappings':
The tool provides those boost mappings (maybe, a bit outdated):
PS2, Line 29: For this, CMake version of 3.3 and higher is required.
> Doesn't Kudu require a higher version anyway? So maybe this recommendation
It's a good point, I think it's better to remove this.
Line 34: #
> Why does this have to be an absolute path? You could make it absolute using
Because that path is put into the generated GNU makefiles. I wish it were
possible to use a relative one, but that didn't work.
Yes, that can be achieved using that command-line. I just wanted to stress the
path should be an absolute one. I'll keep that wording and add the path
specification using `pwd`, as suggested. Thanks!
Line 39: # -DCMAKE_BUILD_TYPE=debug \
> Isn't this the default value? Can be omitted?
That was point of my concern -- in some places we use NDEBUG directives, and it
might be worth to run the tool in the RELEASE configuration.
But as for the example -- yes, it can be omitted.
PS2, Line 47: <desired_number_of_parallel_jobs>
> Just replace with $(nproc)
PS2, Line 56: to
> drop this
PS2, Line 56: the
> and this
Line 645: in_ctx = 1
> I think I understand what in_ctx does; what does this do?
Oh, that's gone. Will publish a new one soon.
PS2, Line 648: ~
> Isn't this a regex match? Why not a string comparison?
It's not exact match because the path in the IWYU output is the absolute one.
So, here is the catch: I don't want to introduce the notion of the KUDU_ROOT in
this script, because it would be necessary to pass it around, etc. Otherwise,
I would just use the key as is (which is much faster, especially if the number
of elements in the 'muted' array is significant).
However, the 'muted' array should be very short or even empty further down the
road. So, I decided to go this way.
But it might be faster just to check for the 'suffix substring' relationship
instead of generic regex, right. I'll give it a try.
To view, visit http://gerrit.cloudera.org:8080/7604
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Owner: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <danburk...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>