Re: [C++] BUILD_WARNING_LEVEL=EVERYTHING?
After spending a few hours trying to fix the warnings I think I've come around to your points of view :) What got me in the end is DCHECK macros aren't allowed in headers (and I think if we want to clean these up, we should DCHECK before doing casts). Cheers, Micah On Sun, Mar 3, 2019 at 8:11 PM Wes McKinney wrote: > No opposition from me > > On Sun, Mar 3, 2019 at 10:02 PM Micah Kornfield > wrote: > > > > I'm ok with that. I think some of the conversion warnings might be > useful > > (I know I've had bugs in other code that would have been caught with > > them). Would people be opposed if I tried to go through and cleanup the > > EVERYTHING warnings even if more might creep in? > > > > Thanks, > > Micah > > > > On Sun, Mar 3, 2019 at 3:27 PM Wes McKinney wrote: > > > > > I'm of the same mind as Antoine on this. I think it's useful to look > > > at the EVERYTHING warnings periodically, but it is enough effort to > > > keep things simultaneously building cleanly with gcc, clang, and MSVC, > > > that I would prefer to maintain the status quo until it can be > > > demonstrated to be a problem (and even then, it just might be that we > > > add more specific warnings that we care about to the CHECKIN warning > > > level). The clang CHECKIN warnings catch some definitely bad things > > > like missing virtual dtors etc. > > > > > > - Wes > > > > > > On Sun, Mar 3, 2019 at 3:38 AM Antoine Pitrou > wrote: > > > > > > > > > > > > Hmm... There are enough warnings that need pampering in the default > > > > settings that I don't think we want to go the full length of enabling > > > > all warnings. Sometimes it's a PITA to get code to compile cleanly > on > > > > all platforms. > > > > > > > > If compiler writers had a more reasonable judgement when it comes to > > > > designing and enabling warnings, I would perhaps revise my position > ;-) > > > > > > > > Regards > > > > > > > > Antoine. > > > > > > > > > > > > Le 03/03/2019 à 04:47, Micah Kornfield a écrit : > > > > > As part of trying to fix the mingw C++ build [1], I tried compiling > > > with > > > > > BUILD_WARNING_LEVEL=EVERYTHING and it seems like it highlights a > lot of > > > > > possible warnings that aren't in CHECKIN. Have we not turned on > the > > > > > additional warnings because there was too much to fix at the time > this > > > was > > > > > added? Or is a conscious decision to ignore some warnings? > > > > > > > > > > Thanks, > > > > > Micah > > > > > > > > > > [1] https://github.com/apache/arrow/pull/3793 > > > > > > > > >
Re: [C++] BUILD_WARNING_LEVEL=EVERYTHING?
No opposition from me On Sun, Mar 3, 2019 at 10:02 PM Micah Kornfield wrote: > > I'm ok with that. I think some of the conversion warnings might be useful > (I know I've had bugs in other code that would have been caught with > them). Would people be opposed if I tried to go through and cleanup the > EVERYTHING warnings even if more might creep in? > > Thanks, > Micah > > On Sun, Mar 3, 2019 at 3:27 PM Wes McKinney wrote: > > > I'm of the same mind as Antoine on this. I think it's useful to look > > at the EVERYTHING warnings periodically, but it is enough effort to > > keep things simultaneously building cleanly with gcc, clang, and MSVC, > > that I would prefer to maintain the status quo until it can be > > demonstrated to be a problem (and even then, it just might be that we > > add more specific warnings that we care about to the CHECKIN warning > > level). The clang CHECKIN warnings catch some definitely bad things > > like missing virtual dtors etc. > > > > - Wes > > > > On Sun, Mar 3, 2019 at 3:38 AM Antoine Pitrou wrote: > > > > > > > > > Hmm... There are enough warnings that need pampering in the default > > > settings that I don't think we want to go the full length of enabling > > > all warnings. Sometimes it's a PITA to get code to compile cleanly on > > > all platforms. > > > > > > If compiler writers had a more reasonable judgement when it comes to > > > designing and enabling warnings, I would perhaps revise my position ;-) > > > > > > Regards > > > > > > Antoine. > > > > > > > > > Le 03/03/2019 à 04:47, Micah Kornfield a écrit : > > > > As part of trying to fix the mingw C++ build [1], I tried compiling > > with > > > > BUILD_WARNING_LEVEL=EVERYTHING and it seems like it highlights a lot of > > > > possible warnings that aren't in CHECKIN. Have we not turned on the > > > > additional warnings because there was too much to fix at the time this > > was > > > > added? Or is a conscious decision to ignore some warnings? > > > > > > > > Thanks, > > > > Micah > > > > > > > > [1] https://github.com/apache/arrow/pull/3793 > > > > > >
Re: [C++] BUILD_WARNING_LEVEL=EVERYTHING?
I'm ok with that. I think some of the conversion warnings might be useful (I know I've had bugs in other code that would have been caught with them). Would people be opposed if I tried to go through and cleanup the EVERYTHING warnings even if more might creep in? Thanks, Micah On Sun, Mar 3, 2019 at 3:27 PM Wes McKinney wrote: > I'm of the same mind as Antoine on this. I think it's useful to look > at the EVERYTHING warnings periodically, but it is enough effort to > keep things simultaneously building cleanly with gcc, clang, and MSVC, > that I would prefer to maintain the status quo until it can be > demonstrated to be a problem (and even then, it just might be that we > add more specific warnings that we care about to the CHECKIN warning > level). The clang CHECKIN warnings catch some definitely bad things > like missing virtual dtors etc. > > - Wes > > On Sun, Mar 3, 2019 at 3:38 AM Antoine Pitrou wrote: > > > > > > Hmm... There are enough warnings that need pampering in the default > > settings that I don't think we want to go the full length of enabling > > all warnings. Sometimes it's a PITA to get code to compile cleanly on > > all platforms. > > > > If compiler writers had a more reasonable judgement when it comes to > > designing and enabling warnings, I would perhaps revise my position ;-) > > > > Regards > > > > Antoine. > > > > > > Le 03/03/2019 à 04:47, Micah Kornfield a écrit : > > > As part of trying to fix the mingw C++ build [1], I tried compiling > with > > > BUILD_WARNING_LEVEL=EVERYTHING and it seems like it highlights a lot of > > > possible warnings that aren't in CHECKIN. Have we not turned on the > > > additional warnings because there was too much to fix at the time this > was > > > added? Or is a conscious decision to ignore some warnings? > > > > > > Thanks, > > > Micah > > > > > > [1] https://github.com/apache/arrow/pull/3793 > > > >
Re: [C++] BUILD_WARNING_LEVEL=EVERYTHING?
I'm of the same mind as Antoine on this. I think it's useful to look at the EVERYTHING warnings periodically, but it is enough effort to keep things simultaneously building cleanly with gcc, clang, and MSVC, that I would prefer to maintain the status quo until it can be demonstrated to be a problem (and even then, it just might be that we add more specific warnings that we care about to the CHECKIN warning level). The clang CHECKIN warnings catch some definitely bad things like missing virtual dtors etc. - Wes On Sun, Mar 3, 2019 at 3:38 AM Antoine Pitrou wrote: > > > Hmm... There are enough warnings that need pampering in the default > settings that I don't think we want to go the full length of enabling > all warnings. Sometimes it's a PITA to get code to compile cleanly on > all platforms. > > If compiler writers had a more reasonable judgement when it comes to > designing and enabling warnings, I would perhaps revise my position ;-) > > Regards > > Antoine. > > > Le 03/03/2019 à 04:47, Micah Kornfield a écrit : > > As part of trying to fix the mingw C++ build [1], I tried compiling with > > BUILD_WARNING_LEVEL=EVERYTHING and it seems like it highlights a lot of > > possible warnings that aren't in CHECKIN. Have we not turned on the > > additional warnings because there was too much to fix at the time this was > > added? Or is a conscious decision to ignore some warnings? > > > > Thanks, > > Micah > > > > [1] https://github.com/apache/arrow/pull/3793 > >
Re: [C++] BUILD_WARNING_LEVEL=EVERYTHING?
Hmm... There are enough warnings that need pampering in the default settings that I don't think we want to go the full length of enabling all warnings. Sometimes it's a PITA to get code to compile cleanly on all platforms. If compiler writers had a more reasonable judgement when it comes to designing and enabling warnings, I would perhaps revise my position ;-) Regards Antoine. Le 03/03/2019 à 04:47, Micah Kornfield a écrit : > As part of trying to fix the mingw C++ build [1], I tried compiling with > BUILD_WARNING_LEVEL=EVERYTHING and it seems like it highlights a lot of > possible warnings that aren't in CHECKIN. Have we not turned on the > additional warnings because there was too much to fix at the time this was > added? Or is a conscious decision to ignore some warnings? > > Thanks, > Micah > > [1] https://github.com/apache/arrow/pull/3793 >
