sendyne-nicocvn
commented
6 years ago Closed hak8or closed 6 years ago
sendyne-nicocvn
commented
6 years ago 
hak8or
commented
6 years ago If the issue is that other compilers were not error-ing when they should be, maybe it would be worthwhile to file a bug report for clang and a regression in gcc? A simpler case would be needed though, in which case I would suggest creating an issue for it.
On Feb 22, 2018 5:04 PM, "Sendyne Principal Scientist" < notifications@github.com> wrote:
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/sendyne/cppreg/issues/2#issuecomment-367838181, or mute the thread https://github.com/notifications/unsubscribe-auth/AA5gCqyCPO7JZy32qkCA_wcsjoq2XuK2ks5tXeSCgaJpZM4SPxFh .
sendyne-nicocvn
commented
6 years ago This is a bit long ... my original explanation was inadequate so here is the full story.
The issue is a bit more ... subtle (and unfortunately this manifests itself as a linking issue so godbolt is not a good candidate for investigating).
Consider the following code:
#include <vector>
struct NS {
constexpr static const int n = 1;
};
int main() {
std::vector<int> v(10, NS::n);
}At first glance it should compile fine, but compilation with GCC or Clang (I only tested x86 and ARM) using C++11 or C++14 will fail (C++17 and above will compile; more on that later) because of an undefined reference for the symbol NS::n. A few remarks:
std::vector<int> v(NS::n, 10) will compile (optimization or not).Now a few comments/explanations ...
In the original code:
int main() {
std::vector<int> v(10, NS::n); // ctor: vector(size_type count,const T&)
}we take a const reference to NS::n and that is sufficient for it to be ODR-used [1]. And therefore the following from C++11 standard applies: If a const static data member or a constexpr static data member (since C++11) is odr-used, a definition at namespace scope is still required, but it cannot have an initializer. This definition is deprecated for constexpr data members (since C++17) [2].
So:
This is why the original code compiles for C++17 and above.
The following fix is required in the original code:
struct NS {
constexpr static const int n = 1;
};
const int NS::n; // now it will be resolved during linkingThis also explains why std::vector<int> v(NS::n, 10) would compile without the fix: the first argument to the vector constructor is by value and that does not make NS::n ODR-used.
Enabling optimization was fixing the problem because the const reference step was (most likely) wiped away.
Now let's go back to cppreg original issue.
cppregThe issue was exactly about that.
This was the code prior to 11734b0bf48b926a5f25d8db9e5b8f471fb48e35:
template <typename Register>
struct Shadow<Register, true> {
static typename Register::type value;
constexpr static const bool use_shadow = true;
};
template <typename Register>
typename Register::type Shadow<Register, true>::value = Register::reset;
template <typename Register>
const bool Shadow<Register, true>::use_shadow;The twist in the story is that the namespace scope definition of use_shadow was not due to cppreg. Some unit testing code (not part of the project) was taking a const rerence to use_shadow thus making it ODR-used thus requiring the definition.
So this is not a (re)definition nor a re-definition. And this is valid C++11 code.
Regarding the original issue:
use_shadow will fail to compile (linker error); I do not think this is an important issue as this is an internal member (i.e., not part of the documented API).A link to this discussion should be included in the code for reference purposes.
Question: do we want to support GCC 4.8.5? If yes, 11734b0bf48b926a5f25d8db9e5b8f471fb48e35 is a fix. If not, we should revert the change.
hak8or
commented
6 years ago Opened again because there is still discussion.
Fantastic writeup! I will re-read this later tomorrow to make sure I am understanding it correctly, but I am still not quite clear on why the discrepancy across GCC versions even though the same flags saying to use C++11 were used. Is it due to a differing interpretation of what you copied from within 9.4 Static members [class.static] across the GCC versions?
Since I see the fix (11734b0bf48b926a5f25d8db9e5b8f471fb48e35) has been committed to master, I assume everything is still compiling and therefore the mentioned side effect does/did not present any large issues regarding unit tests?
GCC 4.9 seems to be from August 3, 2016 and GCC 4.8.5 is from June 23rd, 2015, so it's not that old, and therefore I do think it's pertinent to have it working. Furthermore, as you said, use_shadow is for internal purposes only, so this change wouldn't be considered breaking anyways and therefore I say 11734b0bf48b926a5f25d8db9e5b8f471fb48e35 should be kept.
sendyne-nicocvn
commented
6 years ago Since I see the fix (11734b0) has been committed to master, I assume everything is still compiling and therefore the mentioned side effect does/did not present any large issues regarding unit tests?
No significant issue. The only one was (Googletest is used for unit testing) related to a test using:
EXPECT_TRUE(some_reg_with_shadow::shadow::use_shadow);Under the hood EXPECT_TRUE evaluates to a type named AssertionResult which will copy construct from a bool (and hence taking a const reference and hence triggering the linking error). This was fixed with:
EXPECT_TRUE(bool(some_reg_with_shadow::shadow::use_shadow));This performs a lvalue-to-rvalue conversion and therefore use_shadow is not ODR-used anymore.
Is it due to a differing interpretation of what you copied from within 9.4 Static members [class.static] across the GCC versions?
I am not really sure. There is the exact same issue on SO here. And the latest recipient of the Draper prize agrees that our code prior to the fix is valid.
However ... a glimmer of hope.
This:
struct NS {
constexpr static const int n = 1;
};
const int NS::n;fails to compile with GCC 4.8.5 (Godbolt) but the error message complains about the fact the re-declaration differs in constexpr-ness. So this is apparently bogus and we also know from the discussion above that this is valid code.
The fix is:
#include <vector>
struct NS {
constexpr static const int n = 1;
};
constexpr const int NS::n;
int main() {
std::vector<int> v(10, NS::n);
};This compiles and runs with GCC 4.8.5 (Clang was always fine by the way).
This will be implemented shortly and tested.
As of 5cf8a851d879d96790d26dadd9cc5ca2af38874c it seems we fail to compile on PowerPC GCC 4.8.5 according to GodBolt as shown here.
with the following compiler error: