[native-toolchain-CR] IMPALA-5174: Add hidden flags to gflags (2.2.0-p1)

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5174: Add hidden flags to gflags (2.2.0-p1)
......................................................................


Patch Set 1:

(3 comments)

longer term, is it reasonable to think we'd try to get this functionality 
upstream?

http://gerrit.cloudera.org:8080/#/c/6672/1/source/gflags/gflags-2.2.0-patches/0001-Allow-hidden-flags-e.g.-DEFINE_int32_hidden.patch
File 
source/gflags/gflags-2.2.0-patches/0001-Allow-hidden-flags-e.g.-DEFINE_int32_hidden.patch:

Line 5: 
we should have something to explain this since this isn't tracking an upstream 
change and we don't have a JIRA to track this


PS1, Line 191: +#define DEFINE_int32_hidden(name, val, txt)                     
        \
             : +  DEFINE_VARIABLE(GFLAGS_NAMESPACE::int32, I, name, val, txt, 
true)
missing DEFINE_uint32_hidden


PS1, Line 239: +  namespace fLS {                                               
            \
             : +    using ::fLS::clstring;                                      
            \
             : +    using ::fLS::StringFlagDestructor;                          
            \
             : +    static union { void* align; char s[sizeof(clstring)]; } 
s_##name[2];    \
             : +    clstring* const FLAGS_no##name = ::fLS::                    
            \
             : +                                   
dont_pass0toDEFINE_string(s_##name[0].s, \
             : +                                                             
val);          \
             : +    static GFLAGS_NAMESPACE::FlagRegisterer o_##name(           
            \
             : +        #name, MAYBE_STRIPPED_HELP(txt), __FILE__,              
            \
             : +        FLAGS_no##name, new (s_##name[1].s) 
clstring(*FLAGS_no##name),      \
             : +        true);                                                  
            \
             : +    static StringFlagDestructor d_##name(s_##name[0].s, 
s_##name[1].s);     \
             : +    extern GFLAGS_DLL_DEFINE_FLAG clstring& FLAGS_##name;       
            \
             : +    using fLS::FLAGS_##name;                                    
            \
             : +    clstring& FLAGS_##name = *FLAGS_no##name;                   
            \
             : +  }                                                             
            \
             : +  using fLS::FLAGS_##name
There's a bunch of code here that will be hard to identify if it starts 
diverging from DEFINE_string (not hidden). I'm worried about the supportability 
of this patch. Any thoughts about how we can avoid issues moving forward?


-- 
To view, visit http://gerrit.cloudera.org:8080/6672
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I48d718bac3dbf548cdaefc70f8f497bbebe30da6
Gerrit-PatchSet: 1
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com>
Gerrit-HasComments: Yes