Alexey Serbin has posted comments on this change.

Change subject: [build-support] added IWYU filter script
......................................................................


Patch Set 2:

(12 comments)

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.

http://gerrit.cloudera.org:8080/#/c/7604/2/build-support/iwyu/iwyu-filter.awk
File build-support/iwyu/iwyu-filter.awk:

PS2, Line 19: include-what-you-use
> "include-what-you-use (IWYU) tool"
Done


PS2, Line 20: for
> of
Done


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 
special
            : # 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':
  
https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/IWYUMappings.md

The tool provides those boost mappings (maybe, a bit outdated):
  
https://github.com/include-what-you-use/include-what-you-use/blob/master/boost-all.imp


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: #     
IWYU=<abs_path_kudu>/thirdparty/clang-toolchain/bin/include-what-you-use
> 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)
Done


PS2, Line 56: to
> drop this
Done


PS2, Line 56: the
> and this
Done


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-MessageType: comment
Gerrit-Change-Id: Idae8dae3e488151590d5420adc1f0a084339e2fa
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
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>
Gerrit-HasComments: Yes

Reply via email to