Re: [lmi] [PATCH] Fixes for clang compilation

[Vadim Zeitlin]

On Fri, 7 Apr 2017 00:19:26 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2017-04-06 00:06, Vadim Zeitlin wrote:
...
GC> >  The other one fixes a warning given by clang for the unused private
GC> > fields. This one could be disabled instead of fixing it if there is some
GC> > really good reason to keep these fields
GC> 
GC> Could we just disable the warnings instead? Does this patch work?
GC> 
GC> 
---------8<--------8<--------8<--------8<--------8<--------8<--------8<-------
GC> diff --git a/input_sequence_parser.cpp b/input_sequence_parser.cpp
GC> index a66c1a5..0a5fe8d 100644
GC> --- a/input_sequence_parser.cpp
GC> +++ b/input_sequence_parser.cpp
GC> @@ -50,6 +50,8 @@ SequenceParser::SequenceParser
GC>      ,allowed_keywords_              (a_allowed_keywords)
GC>      ,keywords_only_                 (a_keywords_only)
GC>  {
GC> +    stifle_warning_for_unused_value(inforce_duration_);
GC> +    stifle_warning_for_unused_value(effective_year_);
GC>      sequence();
GC>      diagnostic_messages_ = diagnostics_.str();
GC>  }
GC> 
--------->8-------->8-------->8-------->8-------->8-------->8-------->8-------

 Yes, it does work, but I just don't see any point in keeping these unused
private members. I.e. it's not only about fixing the clang warning (which
is important, of course), but also, and arguably even more importantly,
about avoiding confusion due to not being able to understand where are
these fields used when examining the code later. I guess this latter goal
could also be achieved by writing a comment before the lines added above
explaining that these fields are not used at all, but I have trouble seeing
how would such a comment rationalize still keeping these fields in the
first place. Removing them just seems simpler and more straightforward.

GC> > or, on the contrary, extended in
GC> > order to remove the SequenceParser ctor arguments corresponding to them,
GC> 
GC> But if we do that, then we should remove them from class InputSequence too,
GC> and then from InputSequenceEntry, and from 'input_realization.cpp'...

 I still think that it would be better to do all this, but I understand,
and agree, that it's hardly a priority, so I accept that we're going to
keep unused parameters in the ctor. But unused parameters are common and,
IMHO, much less surprising than entirely unused (but assigned to) fields.

GC> and then we'd have to add them back in if we ever want to use them.

 Which is just a simple "git revert" away...
VZ