Re: [PATCHv3] gdb: style filenames in separate debug file warnings

[Andrew Burgess <aburgess@redhat.com>]

Simon Marchi <simark@simark.ca> writes:

> On 7/7/23 11:03, Andrew Burgess wrote:
>> Simon Marchi <simark@simark.ca> writes:
>> 
>>>> @@ -209,7 +210,7 @@ build_id_to_exec_bfd (size_t build_id_len, const bfd_byte *build_id)
>>>>  
>>>>  std::string
>>>>  find_separate_debug_file_by_buildid (struct objfile *objfile,
>>>> -				     std::vector<std::string> *warnings_vector)
>>>> +				     deferred_warnings *warnings)
>>> Reference instead of pointer?
>> 
>> I thought at one point (maybe I'm miss-remembering) GDB style was to
>> pass a pointer if an object was modified, and a const reference when an
>> object was not going to be modified.  I've stuck to that since pretty
>> much forever for GDB code .... is this not (or was it never) a thing?
>> 
>> I've not changed this for now, but I can do this if that's your
>> preference, just let me know.
>
> I had added it to the wiki for a brief period of time, but removed it
> since there was some disagreement (not in the thread, it must have been
> offline, on IRC, or in another thread):
>
> https://inbox.sourceware.org/gdb-patches/b31a374788e2b6067786fb8c6c0a3acc@polymtl.ca/
>
> I based myself on the Google style guide at the time, which I thought
> had a good rationale.  But they have since changed it to allow non-const
> references:
>
> https://github.com/google/styleguide/commit/7a7a2f510efe7d7fc
>
> Since then I've gotten used to it, and use non-const references when
> needed, since that's just the C++ way to do it.
>
>>>> @@ -1274,11 +1273,10 @@ elf_symfile_read_dwarf2 (struct objfile *objfile,
>>>>  		}
>>>>  	    }
>>>>  	}
>>>> -      /* If all the methods to collect the debuginfo failed, print
>>>> -	 the warnings, if there're any. */
>>>> -      if (debugfile.empty () && !has_dwarf2 && !warnings_vector.empty ())
>>>> -	for (const std::string &w : warnings_vector)
>>>> -	  warning ("%s", w.c_str ());
>>>> +      /* If all the methods to collect the debuginfo failed, print the
>>>> +	 warnings, if there are any. */
>>>> +      if (debugfile.empty () && !has_dwarf2 && !warnings.empty ())
>>>> +	warnings.emit ();
>>>
>>> The warnings.empty() seems unnecessary to me, since emit will be a no-op
>>> if there are no warnings.
>> 
>> Funny you should call this out.  I added the .empty() just prior to
>> posting this to the mailing list because, when I gave the patches a
>> final review, it in someway felt more obvious what was going on if there
>> was an empty check.  But I'm not really tied to it, so I'm going to
>> remove the empty check.
>
> No strong feelings here.  But the "if there are any" in your comment
> makes it clear to me that there might be no warnings to emit.
>
>> An updated version of the patch is below.
>
> I don't really have time right now to look at the updated patch, but if
> all you changed is things I pointed out:
>
> Approved-By: Simon Marchi <simon.marchi@efficios.com>

Pushed.

Thanks,
Andrew