Use inline variables when available #423

gnzlbg · 2016-07-05T10:18:54Z

adds RANGES_INLINE_VARIABLE(type, name); macro to declare inline variables
deprecates static_const utility on all C++ versions
- clients might be using it in their own code => deprecate for a while
- adds a detail::inline_variable_ utility equivalent to static_const that
  is used internally in range-v3 and that clients are not supposed to use

gnzlbg · 2016-07-05T10:23:47Z

I still need to figure out what is going with swap, see here.

Basically the whole library compiles fine if I put the inline variable in the v3 namespace, except for the header /utility/any.hpp, where any defines a friend function swap which collides with the inline variable if they are in the same namespace (because one is a function, and the other one is a variable).

ericniebler · 2016-07-05T15:58:16Z

include/range/v3/utility/inline_variable.hpp

+}
+#endif  // RANGES_CXX_INLINE_VARIABLES
+
+#endif


This is a utility for building static const objects, not inline variables. The name was right before.

ericniebler · 2016-07-05T15:59:45Z

where any defines a friend function swap which collides with the inline variable

Sounds like a clang bug. IIUC, inline variables shouldn't be changing name lookup. Have you reported it?

ericniebler · 2016-07-05T16:33:38Z

include/range/v3/utility/static_const.hpp

-        struct static_const
+        struct
+        RANGES_DEPRECATED("Use RANGES_CXX_INLINE_VARIABLE instead which supports C++17 inline variables")
+        static_const


Suggest undeprecating this and removing inline_variable_.

Sorry about that, I didn't knew if static_const should be deprecated or not. Thought it might be good to alert users that they could be using the macro instead, but static_const works just fine.

gnzlbg · 2016-07-05T16:39:02Z

Sounds like a clang bug. IIUC, inline variables shouldn't be changing name lookup. Have you reported it?

I don't think it is a bug. It actually makes sense to me that the following fails to compile:

void foo();
constexpr int foo = 0; // ERROR: foo re-declared as a different kind of symbol

The following doesn't fail, but I kind of think it should

void foo();
namespace {
  constexpr int foo = 0;  // OK: foo's are "technically" in different namespaces.
}
// which foo do you mean here?

ericniebler · 2016-07-05T16:50:49Z

It actually makes sense to me that the following fails to compile:
void foo();
constexpr int foo = 0; // ERROR: foo re-declared as a different kind of symbol

But that's not the situation. We have:

struct S {
  friend void foo();
};
constexpr int foo = 0;

There should be nothing wrong with that.

gnzlbg · 2016-07-05T16:56:16Z

There should be nothing wrong with that.

But friend void foo(); just puts foo in S namespace, so that collides as well (this already happens without inline variables, static int foo is enough to make that fail to compile).

I removed the inline_variable thing, static_const is back to normal.

ericniebler · 2016-07-05T17:38:27Z

But friend void foo(); just puts foo in S namespace

But this program:

struct S {
    friend void foo() {}
};

int main() {
    foo();
}

Gives me:

error: 'foo' was not declared in this scope
     foo();
         ^

I thought friend name injection stopped being a thing a long time ago?

gnzlbg · 2016-07-05T18:05:46Z

Indeed. No idea.

@CaseyCarter ?

CaseyCarter · 2016-07-06T00:50:38Z

hidden friends are not found by non-ADL lookups, but the entities they name still exist, and C++ forbids two different entities with the same name unless they are both functions and/or function templates ([basic.scope.declarative]). This program, e.g.,:

struct S {
    friend void foo() {}
};

int foo;

int main() {}

Will blow up any compiler you point it at because ::foo can't be both a (visible) object and a (hidden) function.

[basic.scope.declarative]/1 seems to imply the opposite:

Every name is introduced in some portion of program text called a declarative region, which is the largest part of the program in which that name is valid, that is, in which that name may be used as an unqualified name [emphasis added] to refer to the same entity. In general, each particular name is valid only within some possibly discontiguous portion of program text called its scope. To determine the scope of a declaration, it is sometimes convenient to refer to the potential scope of a declaration. The scope of a declaration is the same as its potential scope unless the potential scope contains another declaration of the same name. In that case, the potential scope of the declaration in the inner (contained) declarative region is excluded from the scope of the declaration in the outer (containing) declarative region.

Since I cannot refer to the function foo at the point in my program where I declare the integer foo, it would seem the name is not valid, and therefore that point is not part of its potential scope since no other declaration is present. I think this is a wording error, and that the intent is that the potential scope of so-called "hidden friends" is that portion of the program in which a matching declaration actually refers to the same entity. I.e., if the program redeclared foo in the global scope:

struct S {
    friend void foo() {}
};

void foo();

int main() { foo(); }

the formerly hidden friend becomes visible, and is in fact the same function.

The later text in [basic.scope.declarative] hints at this. Para 3:

The names declared by a declaration are introduced into the scope in which the declaration occurs, except that the presence of a friend specifier (11.3), certain uses of the elaborated-type-specifier (7.1.6.3), and using-directives (7.3.4) alter this general behavior.

and the note after para 4:

[ Note: These restrictions apply to the declarative region into which a name is introduced, which is not necessarily the same as the region in which the declaration occurs. In particular, elaborated-type-specifiers (7.1.6.3) and friend declarations (11.3) may introduce a (possibly not visible) name into an enclosing namespace; these restrictions apply to that region. ...

Our customization point design "cheats" around this rule by putting the object declaration inside a different namespace, accessed via a using directive (such as an unnamed namespace):

struct S {
    friend void foo() {}
};

namespace bob { int foo; }
using namespace bob;

int x = foo;

int main() {}

::foo and bob::foo are distinct entities yet all is right with the world. We are taking advantage of the fact that lookup for use of a name has differing properties from lookup to determine if a name is already defined. We should be able to achieve the same effect with an inline variable in a non-unnamed namespace. Clang trunk seems to like:

namespace ranges { inline namespace v3 {
struct S {
    friend void swap(S&, S&) {}
};

namespace CPOs { inline constexpr int swap = 42; }
using namespace CPOs;
}}

int main() { return ranges::swap; }

CaseyCarter · 2016-07-06T00:54:23Z

I suggest something like:

namespace ranges
{
    inline namespace v3
    {
        namespace function_objects {}
        using namespace function_objects;
    }
}

#define RANGES_INLINE_VARIABLE(name, type) \
    namespace function_objects { inline constexpr type name{}; }

pfultz2 · 2016-07-06T00:59:10Z

We should be able to achieve the same effect with an inline variable in a non-unnamed namespace. Clang trunk seems to like:

It seems like swap wouldn't have the same address across translation units due to the anonymous namespace.

CaseyCarter · 2016-07-06T01:01:24Z

It seems like swap wouldn't have the same address across translation units due to the anonymous namespace.

Apologies for using the ridiculously confusing phrase "non-unnamed namespace" - I meant a normal, named namespace like CPOs in the example.

pfultz2 · 2016-07-06T01:20:03Z

Also, you could try defining it like this:

#if RANGES_CXX_INLINE_VARIABLES < RANGES_CXX_INLINE_VARIABLES_17
#   if defined(__GNUC__) || defined (__clang__)
#       define RANGES_INLINE_VAR extern __attribute__((weak))
#   elif defined(_MSC_VER)
#       define RANGES_INLINE_VAR extern __declspec(selectany)
#   else // Your compiler is hopeless
#       define RANGES_INLINE_VAR static
#   endif
#else
#    define RANGES_INLINE_VAR inline
#endif

Then just do:

namespace function_objects { RANGES_INLINE_VAR constexpr adl_swap_detail::swap_fn swap{}; }

On gcc and clang this should keep ABI compatibility between C++17 and earlier versions, if that were to matter. Of course, __declspec(selectany) doesn't work with constexpr yet. However, it may not matter, since range-v3 doesn't work with MSVC yet either.

gnzlbg · 2016-07-06T08:41:31Z

The last commit:

adds function_objects namespaces and puts the inline variables in them per @CaseyCarter suggestion,
the variables are marked inline since, as @CaseyCarter mentions in clang bug 28385, the approved "inline variables paper" is P0386R2 (not P0386R0 which is the only one available), and there constexpr doesn't always imply inline (as opposed to in P0386R0): it only implies inline for static data members (and not for variables in namespace scope).

I've tested this PR with the changes of PR #426 and everything works fine on clang-trunk in C++11/14/1z.

Still, I'd block merging this PR till something like #426 gets merged and we start testing clang-trunk on travis again.

Question: I don't like this:

        namespace function_objects {}
        using namespace function_objects;

        namespace aux
        {
            namespace function_objects {}
            using namespace function_objects;
        }

        namespace view
        {
          namespace function_objects {}
          using namespace function_objects;
        }

        namespace action
        {
          namespace function_objects {}
          using namespace function_objects;
        }

        namespace detail
        {
          namespace function_objects {}
          using namespace function_objects;
        }

but, does anybody have a better idea? I think I'd rather use an anonymous namespace in the macro:

#define RANGES_INLINE_VARIABLE_(type, name, inline_) \
    namespace                                        \
    {                                                \
        inline constexpr type name{};                \
    }

ericniebler · 2016-07-06T23:06:11Z

I think I'd rather use an anonymous namespace in the macro

Does that work? If not, then pls squash this and I'll commit. If so, I think that would be an improvement.

CaseyCarter · 2016-07-06T23:09:27Z

I think I'd rather use an anonymous namespace in the macro

If you put the definitions in unnamed namespaces, they define distinct objects in each TU, in which case there's no reason to declare them inline.

EDIT: And formally, we get ODR violations.

gnzlbg · 2016-07-07T08:44:19Z

@ericniebler squashed, let's wait till it passes the tests under clang-trunk.

@CaseyCarter Yes, forgot about that. That was a pointless suggestion. We really do need some named namespace.

CaseyCarter · 2016-07-07T15:11:49Z

I admit I'm not a fan of the name function_objects despite it being my suggestion. The name isn't significant in itself, it's just something that will appear in error messages where ::<anonymous>:: used to be. We can trivially change it whenever we decide on a better color for the bikeshed.

CaseyCarter · 2016-07-07T16:11:34Z

include/range/v3/detail/config.hpp

+
+#if RANGES_CXX_INLINE_VARIABLES < RANGES_CXX_INLINE_VARIABLES_17
+#define RANGES_INLINE_VARIABLE_(type, name, inline_)      \
+    inline_ namespace                                     \


^^ inline_ is used here.

- adds RANGES_INLINE_VARIABLE(type, name); macro to declare inline variables - deprecates static_const utility on all C++ versions - we should deprecate this for a while since clients might be using it in their own code - adds a detail::inline_variable_ utility equivalent to static_const that is used internally in range-v3 and that clients are not supposed to use

CaseyCarter · 2016-08-04T03:46:49Z

@CaseyCarter How can stuff in an unnamed namespace violate ODR?

When external linkage entities with definitions in multiple TUs refer to different entities with internal linkage using the same name:

constexpr int foo = 42;
inline int& f() { return foo; }

gnzlbg force-pushed the inline_variables branch 4 times, most recently from a25a224 to 964ff9b Compare July 5, 2016 15:56

ericniebler reviewed Jul 5, 2016
View reviewed changes

gnzlbg force-pushed the inline_variables branch from 964ff9b to a755382 Compare July 5, 2016 16:53

gnzlbg force-pushed the inline_variables branch from d1c82ce to 07197c0 Compare July 7, 2016 08:40

CaseyCarter reviewed Jul 7, 2016
View reviewed changes

gnzlbg force-pushed the inline_variables branch 2 times, most recently from 4182dc6 to 4cfcb59 Compare July 8, 2016 18:26

ericniebler merged commit 5018993 into ericniebler:master Jul 9, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use inline variables when available #423

Use inline variables when available #423

gnzlbg commented Jul 5, 2016

gnzlbg commented Jul 5, 2016

ericniebler Jul 5, 2016

ericniebler commented Jul 5, 2016 •

edited

Loading

ericniebler Jul 5, 2016

gnzlbg Jul 5, 2016 •

edited

Loading

gnzlbg commented Jul 5, 2016

ericniebler commented Jul 5, 2016

gnzlbg commented Jul 5, 2016

ericniebler commented Jul 5, 2016

gnzlbg commented Jul 5, 2016

CaseyCarter commented Jul 6, 2016

CaseyCarter commented Jul 6, 2016

pfultz2 commented Jul 6, 2016

CaseyCarter commented Jul 6, 2016

pfultz2 commented Jul 6, 2016

gnzlbg commented Jul 6, 2016 •

edited

Loading

ericniebler commented Jul 6, 2016

CaseyCarter commented Jul 6, 2016 •

edited

Loading

gnzlbg commented Jul 7, 2016

CaseyCarter commented Jul 7, 2016

CaseyCarter Jul 7, 2016

CaseyCarter commented Aug 4, 2016

Use inline variables when available #423

Use inline variables when available #423

Conversation

gnzlbg commented Jul 5, 2016

gnzlbg commented Jul 5, 2016

ericniebler Jul 5, 2016

Choose a reason for hiding this comment

ericniebler commented Jul 5, 2016 • edited Loading

ericniebler Jul 5, 2016

Choose a reason for hiding this comment

gnzlbg Jul 5, 2016 • edited Loading

Choose a reason for hiding this comment

gnzlbg commented Jul 5, 2016

ericniebler commented Jul 5, 2016

gnzlbg commented Jul 5, 2016

ericniebler commented Jul 5, 2016

gnzlbg commented Jul 5, 2016

CaseyCarter commented Jul 6, 2016

CaseyCarter commented Jul 6, 2016

pfultz2 commented Jul 6, 2016

CaseyCarter commented Jul 6, 2016

pfultz2 commented Jul 6, 2016

gnzlbg commented Jul 6, 2016 • edited Loading

ericniebler commented Jul 6, 2016

CaseyCarter commented Jul 6, 2016 • edited Loading

gnzlbg commented Jul 7, 2016

CaseyCarter commented Jul 7, 2016

CaseyCarter Jul 7, 2016

Choose a reason for hiding this comment

CaseyCarter commented Aug 4, 2016

ericniebler commented Jul 5, 2016 •

edited

Loading

gnzlbg Jul 5, 2016 •

edited

Loading

gnzlbg commented Jul 6, 2016 •

edited

Loading

CaseyCarter commented Jul 6, 2016 •

edited

Loading