On Mon, Nov 9, 2015 at 12:17 AM, Jeff Law <l...@redhat.com> wrote: > On 11/07/2015 07:31 AM, Pedro Alves wrote: >> >> Hi Richard, >> >> Passerby comment below. >> >> On 11/07/2015 01:21 PM, Richard Sandiford wrote: >>> >>> -/* Lookup the identifier ID. */ >>> +/* Lookup the identifier ID. Allow "null" if ALLOW_NULL. */ >>> >>> id_base * >>> -get_operator (const char *id) >>> +get_operator (const char *id, bool allow_null = false) >>> { >>> + if (allow_null && strcmp (id, "null") == 0) >>> + return null_id; >>> + >>> id_base tem (id_base::CODE, id); >> >> >> Boolean params are best avoided if possible, IMO. In this case, >> it seems this could instead be a new wrapper function, like: > > This hasn't been something we've required for GCC. I've come across this > recommendation a few times over the last several months as I continue to > look at refactoring and best practices for codebases such as GCC. > > By encoding the boolean in the function's signature, it (IMHO) does make the > code a bit easier to read, primarily because you don't have to go lookup the > tense of the boolean). The problem is when the boolean is telling us some > property an argument, but there's more than one argument and other similar > situations. > > I wonder if the real benefit is in the refactoring necessary to do things in > this way without a ton of code duplication.
I think the patch is ok as-is. Thus ok. Thanks, Richard. > Jeff > >