Re: [PATCH 04/40] Fix TAB-completion + .gdb_index slowness (generalize filename_seen_cache)

[Keith Seitz <keiths@redhat.com>]

I just have a little comment...

On 06/02/2017 05:22 AM, Pedro Alves wrote:
> diff --git a/gdb/filename-seen-cache.h b/gdb/filename-seen-cache.h
> new file mode 100644
> index 0000000..c041d3e
> --- /dev/null
> +++ b/gdb/filename-seen-cache.h
> @@ -0,0 +1,64 @@
> +/* Filename-seen cache for the GNU debugger, GDB.
> +
> +   Copyright (C) 1986-2017 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include "defs.h"
> +#include "common/function-view.h"
> +
> +/* Cache to watch for file names already seen.  */
> +
> +class filename_seen_cache
> +{
> +public:
> +  filename_seen_cache ();
> +  ~filename_seen_cache ();
> +
> +  /* Disable copy.  */
> +  filename_seen_cache (const filename_seen_cache &) = delete;
> +  void operator= (const filename_seen_cache &) = delete;
> +
> +  /* Empty the cache, but do not delete it.  */
> +  void clear ();
> +
> +  /* If FILE is not already in the table of files in CACHE, return
> +     false; otherwise return true.  Optionally add FILE to the table
> +     if ADD is true.
> +
> +     NOTE: We don't manage space for FILE, we assume FILE lives as
> +     long as the caller needs.  */
> +  bool filename_seen (const char *file, bool add);

I'm not really a fan of this style of interface. [I realize we've done this for many years.] When reading through code that uses this, e.g.,

   bool seen = cache.filename_seen (file, true);

that "true" doesn't really tell me what that means, and in the context of the method itself, it isn't obvious (to me, at least) what "true" might mean when asking if we've seen a filename. A reader could think that it somehow altered how the search was performed, e.g., case-insensitive or whatnot.

Now if this read

  bool seen = cache.filename_seen (file, ADD);

That reveals much more about what is going to happen.

But that's nitpicky style comment (aka there's no reason to change anything). I didn't otherwise see any issues with the patch.

Keith