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

[Simon Marchi <simark@simark.ca>]

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>

Simon