cplusplus / nbballot

Handling of NB comments in response to ballots
14 stars 4 forks source link

FR-025-017 10.3p6 [module.import] "static inline" used from header units #427

Closed wg21bot closed 1 year ago

wg21bot commented 1 year ago

After implementation and deployment of this rule and the paragraphs (6.3) referenced in the note, it turns out that functions defined “static inline” in headers (often shared between C and C++) are more common than realized and the restriction of not being them to refer such names/entities constitutes a huge practical usability problem.

Permit uses of names/entities defined as “static inline” in header units, and problem that each translation unit that odr-uses those names as a TU-local copy of those definitions, which corresponds to the behaviour if the header units had been #included.

BengtGustafsson commented 1 year ago

Do we have any statistics on whether making functions static inline was ever a conscious choice or just mistakes? To me it seems unlikely that anyone would rely on the addresses of the generated static functions differ. Maybe someone has made a try to increase the cache locality by duplicating the code.

If we can accept that relying on the addresses being different is deprecated header units could allow such declarations but override the actual code duplication. Possibly taking the address of such a function could be made illegal to catch any cases where code relies on address differences but this would cause compilation of valid code taking the address for valid purposes to fail, though.

jfbastien commented 1 year ago

Paper: https://github.com/cplusplus/papers/issues/1356

jfbastien commented 1 year ago

Discussed today, exploring these options:

header <time.h>

struct time_t {/* ... */};

static inline time_t time(time_t *);

Translation unit #1

module;
#include <time.h>                   // With or without include translation
export module A;
static void f() {}
inline void it() { f(); }           // error: is an exposure of f
static inline void its() { f(); }   // OK

void ff() { time(nullptr); }        // OK
inline void iff() { time(nullptr); }// Potential UB: ODR violation.

namespace A {
  export using time = ::time;              // Potential UB: ODR violation.
  export using f = ::f;                    // error: exposure of f
}

Translation unit #2
module;
#include <time.h>
export module B;
namespace B {
  export using time = ::time;
}

TU #3

import A;
import B;

/*
 Don't need to say anything about which `time` you get as we already have
 [module.import]/5 that says you can end up with multiple copies of `time`.

 If a (possibly instantiated) declaration of, or a deduction guide for, a non-TU-local entity in a module interface unit (outside the private-module-fragment, if any) or module partition ([module.unit]) is an exposure <INSERT HERE>, the program is ill-formed. Such a declaration in any other context is deprecated ([depr.local]).

 that names a TU-local entitiy in the purview of a module.

 Also say that: It's unspecified which entity you get for TU-local entities named by an exposure.

 Alternative: Linkage promotion!!!!
 p1498r1
 p1347r1
 p1395r0

 Any reference in a module to a TU-local entitiy that is an exposure creates a copy of that entity with external linkage.
 */

Implementors will discuss within the next month, organize SG2 telecons, and come back with a single suggested resolution by the February 2023 meeting.

erichkeane commented 1 year ago

This was discussed in EWG in the February 7th, 2023 Morning Session in Issaquah. The following poll was taken:

EWG encourages further work in the direction of the presented proposal for FR-025-017, and want(s) to see this again with wording. SF F N A SA
9 10 2 0 0

Result: Consensus

jfbastien commented 1 year ago

Poll: accepts D2808r0 as a resolution to FR-025-017 for C++23, make it a DR on prior versions.

SF F N A SA
0 4 4 7 1

Result: No Consensus

Any objection to unanimous consent to rejecting FR-025-017 for C++23? None.

jensmaurer commented 1 year ago

Rejected. No consensus for a change.