Re: [lmi] Request for code review

[Vadim Zeitlin]

On Mon, 23 Jan 2017 20:11:00 +0000 Greg Chicares <address@hidden> wrote:

GC> Two (tested) changes here. The "auto" change seems obvious.

 Yes, except that I'd use "auto const" but I am, of course, have somewhat
of an obsession with const local variables...

GC> The other is Meyers's "Avoid Duplication in const and Non-const Member
GC> Function" technique. It's already used elsewhere in this file, and I
GC> kind of like it.

 I can't say I like it but I definitely dislike it less than duplicating
code and personally I think it's as good an idea as it ever was. But why
use static_cast<> instead of (strictly less powerful and hence less
dangerous, without mentioning that it's also much clearer) const_cast<>?
I.e. why not do this instead:
---------------------------------- >8 --------------------------------------
diff --git a/any_member.hpp b/any_member.hpp
index 75b600b..a312c4d 100644
--- a/any_member.hpp
+++ b/any_member.hpp
@@ -587,12 +587,9 @@ any_member<ClassType>& 
MemberSymbolTable<ClassType>::operator[]
     (std::string const& s
     )
 {
-    typename member_map_type::iterator i = map_.find(s);
-    if(map_.end() == i)
-        {
-        complain_that_no_such_member_is_ascribed(s);
-        }
-    return i->second;
+    return const_cast<any_member<ClassType>&>
+        (const_cast<MemberSymbolTable<ClassType>const&>(*this).operator[](s)
+        );
 }

 template<typename ClassType>
---------------------------------- >8 --------------------------------------


GC> [0] "I imagine it could be written more simply with C++11"
GC> 
GC> Here's an example that passes the 'any_member' unit test--add these:
GC> 
GC> template<typename T>
GC> inline typename std::add_const<T>::type& constify(T& t)
GC> {
GC>     return t;
GC> }
GC> 
GC> template<typename T>
GC> inline typename std::remove_const<T>::type& unconstify(T& t)
GC> {
GC>     return const_cast<typename std::remove_const<T>::type&>(t);
GC> }
GC> 
GC> and then change the implementation of the non-const operator[] to this:
GC> 
GC>     return unconstify(constify(*this).operator[](s));
GC> 
GC> Probably those [un]constify implementations are woeful in more ways
GC> than I can imagine,

 I don't see anything wrong with them, but I could well be missing
something...

GC> but if they could be done correctly, then the
GC> idiom to avoid duplication could become simple and robust. If this
GC> is a worthwhile idea, then the central idea
GC>   unconstify(constify(*this).Function(Argument))
GC> could presumably be written more concisely by deducing the types of
GC> Function and Argument.

 Maybe, but I'm not sure how would it help to be honest. I.e. we could
write the non-const version as

        return apply_as_nonconst(this, &any_member<ClassType>::operator[], s);

but would this really be better? I don't think so...

 The only other idea I have is to have a common template helper that could
be called from both const and non-const overloads, but I don't really like
the idea of introducing a separate function here.

 So I'd either write it like you did with the helper [un]constify functions
or just use const_cast directly because it could be actually more clear.

 Regards,
VZ