Re: [PATCH] Don't check PST is NULL in read_symtab

[Pedro Alves <palves at redhat dot com>]

On 01/11/2013 03:05 PM, Joel Brobecker wrote:
>> IMO, we don't need an assertion to check PST, because the function is
>> used in this way,
>>
>>   (*pst->read_symtab) (objfile, pst);
> 
> I am fine without the assertion as well. But if we followed your
> argument, we would never need an assertion. For me, assertions
> achieve two goals:
>   1. Clearly document an expectation;
>   2. Cause a semi-friendly abortion, rather than a mysterious behavior
>      or crash.
> As of today, the way this function is called indeed guarantees that
> PST is never NULL. But someone adding a call at a later date might
> introduce a bug and cause it to be called with PST == NULL...

FWIW, I agree with both of you.  I agree with assertion's
roles.  But I also agree with Yao that for functions that implement
a class-like interface and take a "this" pointer, there's no need
to sprinkle the codebase with "gdb_assert (self != NULL)" checks.
BUT (!), when reading one of those functions, it's a bit more obvious
and self-describing that the function takes a "this"-style pointer
when the parameter is actually called "self", and / or at least is
the first parameter in the function's signature.  Like:

static void
dbx_psymtab_to_symtab_1 (struct objfile *objfile, struct partial_symtab *pst)
{

static void
dbx_psymtab_to_symtab_1 (struct partial_symtab *pst, struct objfile *objfile)
{

static void
dbx_psymtab_to_symtab_1 (struct partial_symtab *self, struct objfile *objfile)
{


(It'd be even better for grepability/readability if the implementations
and hook name agreed, like:

- result->read_symtab = dbx_psymtab_to_symtab;
+ result->read_symtab = dbx_read_symtab;

or

- result->read_symtab = dbx_psymtab_to_symtab;
+ result->psymtab_to_symtab = dbx_psymtab_to_symtab;

...

)

-- 
Pedro Alves