Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15722 )
Change subject: [cfile] Add SimpleEncodingTraits to remove boiler plate code in type_encodings.cc ...................................................................... Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/15722/1/src/kudu/cfile/type_encodings.cc File src/kudu/cfile/type_encodings.cc: http://gerrit.cloudera.org:8080/#/c/15722/1/src/kudu/cfile/type_encodings.cc@24 PS1, Line 24: kudu/cfile/binary_dict_block.h > Interesting, why IWYU wants to remove these headers? Any idea? I don't quite understand why IWYU wants to remove these headers and forward declare some of the needed classes in this .cc file but not all. Changes suggested by IWYU lead to build failure. For many of these headers that IWYU suggests removing, this is the only file that includes the *_block.h header file. These headers pretty much declare/define encoder/decoder classes needed in this file and I can perhaps dig more but I don't see much benefit of forward declaring classes in these header for compilation speed. http://gerrit.cloudera.org:8080/#/c/15722/1/src/kudu/cfile/type_encodings.cc@23 PS1, Line 23: : #include "kudu/cfile/binary_dict_block.h" // IWYU pragma: keep : #include "kudu/cfile/binary_plain_block.h" // IWYU pragma: keep : #include "kudu/cfile/binary_prefix_block.h" // IWYU pragma: keep : #include "kudu/cfile/block_encodings.h" : #include "kudu/cfile/bshuf_block.h" // IWYU pragma: keep : #include "kudu/cfile/plain_bitmap_block.h" // IWYU pragma: keep : #include "kudu/cfile/plain_block.h" // IWYU pragma: keep : #include "kudu/cfile/rle_block.h" // IWYU pragma: keep > We need this because IWYU doesn't handle templates well -- is that right? Same as above. http://gerrit.cloudera.org:8080/#/c/15722/1/src/kudu/cfile/type_encodings.cc@57 PS1, Line 57: SimpleEncodingTraits > nit: what about renaming 'SimpleEncodingTraits' --> 'EncodingTraits' ? Done -- To view, visit http://gerrit.cloudera.org:8080/15722 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie01865756713b4a1f75a4e1574080ff892671386 Gerrit-Change-Number: 15722 Gerrit-PatchSet: 1 Gerrit-Owner: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Wed, 15 Apr 2020 19:46:44 +0000 Gerrit-HasComments: Yes
