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
