impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [native-toolchain-CR] IMPALA-5174: Add hidden flags to gflags (2.2.0-p1)
Date Tue, 18 Apr 2017 21:03:23 GMT
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 <henry@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message