Re: [PATCH 2/4 v2] libcpp: Replace macro usage with C++ constructs)

[Jeff Law <law@redhat.com>]

On 05/05/2015 12:21 PM, David Malcolm wrote:
> On Mon, 2015-05-04 at 13:15 -0600, Jeff Law wrote:
> On 05/01/2015 06:56 PM, David Malcolm wrote:
>>>
>>> [I didn't mark the inline functions as "static"; should they be?]
Just a follow-up on this.  I got burned by the ODR issues around 
implicit extern inlines earlier this week.  Basically I had two distinct 
implementations for an inline function with the same name.  They 
obviously appeared in different "header files".

When optimizng, this was fine as they'd actually get inlined and all was 
good.  Things blew up when the optimizer was off.  We got two functions 
with the same name, but different implementations.  The linker blindly 
chose one for me, and in one context it was the wrong one.

This led to bootstrap comparison failures.

So, just be careful :-)

>>
> I moved linemap_assert and linemap_assert_fails higher up within the
> file so that they can appear before the bodies of the inline functions.
> I didn't change their implementation; it's a simple cut-and-paste, so
> these hunks are just an artifact of git's diff-ing algorithm getting
> confused by that.
>
> Should I have called that out in the ChangeLog entries?
It might have helped.  Or you could have pulled it out like you did in 
the follow-up.  No worries.  So consider the issues raised in that code 
as a non-issues for your patch.  They'd be nice things to clean up 
someday though.

>> -#define LINEMAPS_MAPS(SET, MAP_KIND) \
>>> -  (LINEMAPS_MAP_INFO (SET, MAP_KIND))->maps
>>> +inline line_map *
>>> +LINEMAPS_MAPS (const line_maps *set, bool map_kind)
>>> +{
>>> +  return LINEMAPS_MAP_INFO (set, map_kind)->maps;
>>> +}
>>> +
>>> +inline line_map * &
>>> +LINEMAPS_MAPS (line_maps *set, bool map_kind)
>>> +{
>>> +  return LINEMAPS_MAP_INFO (set, map_kind)->maps;
>>> +}
>> Do we really need two implementation of those various functions, one for
>> const line_maps * the other for line_maps *?
>>
>
> The issue here is when they are accessed as lvalues, rather than just as
> rvalues.
>
> I've had a go at eliminating some of the lvalue ones as a followup
> patch: given that after patch 4 these become just field accesses, it's
> trivial to turn the usage sites into plain accesses of fields.
>
> Is that desirable, or are the uses of the macros regarded as useful
> (e.g. for grepping)?  (my preference is for the former: to turn them
> into plain field accesses at those sites)
I think macros would only be useful for those already familiar with the 
code.  If I'm looking for how a particular field gets accessed, I'm 
generally going to be grepping for the field.

Of course, if the field have names that are common ("next" anyone?), 
then, well, grepping for field names is pointless.


>
> Ironically, one of the ones that isn't so easy to eliminate is the
> one you called out above.  Eliminating those ones made the code messier,
> so I kept those ones.
lol.  I just picked one after seeing the pattern repeat a bit.
>
> I'm sending updated patches; the old patch 2 split into 2 parts, and
> a followup ("patch 5 of 4", as it were):
>
>    [PATCH 2/4 v2: part 1] Move linemap_assert higher up within the file
>    [PATCH 2/4 v2: part 2] libcpp: Replace macro usage with C++ constructs
>    [PATCH 5/4] libcpp: Eliminate most of the non-const/reference-returning inline fns
>
> Do I need to do any performance testing of this kit?  Given that the
> resulting inline functions become trivial field lookups in patch 4,
> I'm assuming that our inliner is good enough to optimize it as well
> as the status quo implementation.
Yea, I wouldn't worry about performance of an inline field access vs a 
macro.  We should be good to go.

>
> Also, for comments before functions, do we prefer a blank line between
> the comment and decl, or for them to abutt?
>
> /* Foo.  */
>
> foo ();
>
> vs
>
> /* Bar.  */
> bar ();
>
> How do the following look?
I've done both in my days. I try to follow whatever is in the nearby code.

Let me have a look at the followup...

jeff