[WIP] setAttribute with int/long on OSX & Win by ax3l · Pull Request #333 · openPMD/openPMD-api

ax3l · 2018-09-06T08:12:14Z

Test that writing attributes with non-fixed size types works.

Seems to crash on some environments, such as:

OSX in Travis, long is long long
MSVC, long and int are identical

Problem is, that only one of them is in the variant list and it seems automatic detection that they are the same is not performed in variant on the two obscure OSes above. We should therefore write a trait, that converts types such as int to their current, platform-specific fixed-size type during access, e.g. in Attributable where Attribute(value) is called.

Or in simple words: on OSX and with MSVC on Windows this did not compile:

        long l = 32;
        s.setAttribute("long", l);

since Attribute(value) complains that our Variant does not contain long (but e.g. only int and long long although int is equivalent to long).

First seen here.

anokfireball · 2018-09-06T09:24:00Z

I'm not yet fully grapsing the problem.
My assumption was that datatype derivation in Variant worked similar to what I did manaully in determineDatatype():

using Attribute = Variant< T_dtype_enum, T_one, T_two, /* ... */ >;
T_two two = /* ... */;
Attribute(two).dtype == determineDatatype< T_two >();

Which it probably does, but as

Attribute(T_one()).dtype == Attribute(T_two()).dtype

is apparently true for some platforms and combinations, this might break the logic of correlating datatypes by simply looking up the enum position.

Now to the problem at the pillar:

We should therefore write a trait, that converts types such as int to their current, platform-specific fixed-size type during access

The core of this already exists as run-time behaviour in the form of determineDatatype():
Instead of

template< class T_DTYPES, typename ... T >
class Variant
{
public:
    using resource = variantSrc::variant< T ... >;

    Variant(resource r)
            : dtype{static_cast<T_DTYPES>(r.index())},
              m_data{r}
    { }
} //Variant

we would have to explicitly cast the passed resource

template< class T_DTYPES, typename ... T >
class Variant
{
public:
    using resource = variantSrc::variant< T ... >;

    template< typename U >
    Variant(U&& u)
            : dtype{determineDatatype< U >()},
              m_data{resource(static_cast< YOUR_TRAIT< U >::type >(u))}
    { }
} //Variant

ax3l · 2018-09-06T09:48:09Z

@C0nsultant wrote to you on Gitter, we have a solution

anokfireball · 2018-09-06T11:12:58Z

include/openPMD/backend/Attribute.hpp

+
+        static_assert(
+            wasFound || value,
+            "Type was not found!"


Is this the compilation error one will get when doing something like

struct NotAnAttributeDType { }; Series s = /* ... */; auto attr = NotAnAttributeDType(); s.setAttribute("nonsense", attr);

?

Yes, e.g. on GCC 6.3 you get (I removed the c-string handling for demonstration):

openPMD-api/include/openPMD/backend/Attribute.hpp:97:9: error: static assertion failed: Type was not found! static_assert( ^~~~~~~~~~~~~ In file included from openPMD-api/include/openPMD/Iteration.hpp:23:0, from openPMD-api/include/openPMD/openPMD.hpp:27, from openPMD-api/test/SerialIOTest.cpp:8: openPMD-api/include/openPMD/backend/Attributable.hpp: In instantiation of ‘bool openPMD::Attributable::setAttribute(const string&, const T&) [with T = char [59]; std::__cxx11::string = std::__cxx11::basic_string<char>]’: openPMD-api/test/SerialIOTest.cpp:1290:80: required from here openPMD-api/include/openPMD/backend/Attributable.hpp:234:32: warning: the compiler can assume that the address of ‘value’ will always evaluate to ‘true’ [-Waddress] it->second = Attribute(static_cast<FixedSize const&>(value)); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ openPMD-api/include/openPMD/backend/Attributable.hpp:240:66: warning: the compiler can assume that the address of ‘value’ will always evaluate to ‘true’ [-Waddress] std::make_pair(key, Attribute(static_cast<FixedSize const&>(value))));

anokfireball · 2018-09-06T11:14:14Z

include/openPMD/backend/Attribute.hpp

+     * Takes a fundamental type in T_Input and converts it to its fixed size equivalent.
+     * T_Input is compared against all supported types in @see Attribute .
+     *
+     * Returns the original type of T_Input is already a fixed size type.


Typo?

the original type if T_Input

anokfireball · 2018-09-06T11:18:49Z

test/SerialIOTest.cpp

+        s.setAttribute("vecLong", std::vector< long >({2147483646, 2147483647}));
+        s.setAttribute("vecUShort", std::vector< unsigned short >({65534u, 65535u}));
+        s.setAttribute("vecUInt", std::vector< unsigned int >({65533u, 65531u}));
+        s.setAttribute("vecULong", std::vector< unsigned long >({65532u, 65530u}));


There already is a testcase in CoreTest.cpp that does similar checks. As this is (probably?) not supposed to check the workings in the backend, and rather in Attribute, the tests are better suited there.

Ah dang, no... I moved the test to CoreTest now where a (non-user typical) Attribute() is created directly. I did not fix that yet (see reason below). I fixed the user-interface setAttribute.

ax3l · 2018-09-06T11:58:38Z

thx for the review, addressed all points. Will push the other commits after the test has failed, so I see if the issue is still visible as intended.

anokfireball · 2018-09-06T12:54:24Z

test/CoreTest.cpp

+    // translation from non-fixed size integer types
+    a = Attribute(static_cast< short >(1));
+    REQUIRE((
+        Datatype::INT16 == a.dtype || Datatype::INT32 == a.dtype || Datatype::INT64 == a.dtype


determineDatatype() provides the datatype of explicit fixed width you are looking for. Would be a little easier to read and more explicit.

a = Attribute(static_cast< short >(1)); REQUIRE(determineDatatype< short >() == a.dtype);

Sounds legit, but wouldn't we then test determineDatatype() only against itself?

Anyway, the real test this adds is about the line above, if it compiles:
a = Attribute(static_cast< short >(1));

I just added the REQUIRE line as well for consistency with the existing checks.

wouldn't we then test determineDatatype() only against itself?

No, because Attribute.datatype is not determined by determineDatatype(), but by (std::/mpark::)Variant with a subsequent lookup in the Datatype enum.

ax3l · 2018-09-06T13:15:47Z

@C0nsultant a note on your proposal here: I would like to move the trait into the openPMD::Variant constructor directly because it's more general. The problem is, that we don't have access to all "allowed" types at this point (aka Attribute) since they are set later.

Since this will be a larger refactoring, e.g. unifying Attribute.hpp and Variant.hpp I would leave this open as a possible refactoring later on. For now, I want to get #330 finished with this PR.

ax3l · 2018-09-06T13:19:45Z

@C0nsultant will have to push once more. The test needs to go into SerialTest again since there setAttribute is tested.

ax3l · 2018-09-06T13:33:26Z

test/CoreTest.cpp

    REQUIRE(Datatype::BOOL == a.dtype);
+
+    /* translation from non-fixed size integer types
+     * @todo see


@C0nsultant I will leave this prepared test in here, for #333

ax3l · 2018-09-06T14:14:42Z

include/openPMD/backend/Attribute.hpp

+    template<bool wasFound, typename U, typename T_Head, typename ...T>
+    struct FindTypeImpl
+    {
+        static constexpr bool value = std::is_same<


hm, on the platforms with trouble, is_same will still return false although they are the same... Argh!

turns out they are not the same. each of them are distinct types, just some happen to implement the same precision. int32_t and others are typedef aliases of the funnily named ones (short, int) but not vice versa and not across the latter.

ax3l · 2018-09-06T22:10:24Z

Regarding our underlying issue, this might be interesting reads:
https://github.com/MicrosoftDocs/cpp-docs/issues/411
https://en.cppreference.com/w/cpp/language/types#Integer_types

One could now probably either follow this and go for the fundamental types instead of fixed-size types in all relevant comparisons (e.g. all four short, int, long, long long instead of the three int16_t, int32_t, int64_t) - which is actually what we already do for floating point types - or we add our own is_equivalent_fundamental trait to the functionality inside this PR.

It's also interesting to see that our variadic templates in Attribute change the type in the parameter pack from the fixed size types (which are aliases) to their actual types (e.g. short, int, long long on OSX & Win). This also means there is no cstdint alias for long there.

ax3l · 2018-09-12T14:30:43Z

test/SerialIOTest.cpp

+        s.setAttribute("vecLong", std::vector< long >({2147483646, 2147483647}));
+        s.setAttribute("vecUShort", std::vector< unsigned short >({65534u, 65535u}));
+        s.setAttribute("vecUInt", std::vector< unsigned int >({65533u, 65531u}));
+        s.setAttribute("vecULong", std::vector< unsigned long >({65532u, 65530u}));


read: getAttribute check missing

LongLong missing

Fix access to local `detail` namespace.

Explicitly handle C-strings in order to avoid implicit casts. Makes bindings easier.

Test HDF5/ADIOS1 of writing and reading attributes with non-fixed size types.

ax3l · 2018-09-12T16:06:37Z

This PR is mainly solved with #337.

I will re-open what is left in here (increased coverage, maybe unique C-string signature for set-attribute, maybe the scope fix in one of the tests) into separate PRs.

anokfireball · 2018-09-13T08:42:54Z

include/openPMD/backend/Attributable.hpp

     * @param   value   Value of Attribute stored with the provided key.
     * @return  true if key was already present, false otherwise
+     *
+     * @{


Out of curiosity:
I am unfamiliar with this systax. Does this tell doxygen to generate only one doc for the both of these?

Yes, exactly. It's doxyen group-ing for exactly that use case :)

anokfireball · 2018-09-13T08:53:47Z

test/SerialIOTest.cpp

        s.setAttribute("vecLongdouble", std::vector< long double >({0.L, std::numeric_limits<long double>::max()}));
        s.setAttribute("vecString", std::vector< std::string >({"vector", "of", "strings"}));
-        s.setAttribute("bool", static_cast< unsigned char >(true));
+        // s.setAttribute("bool", true);


See #337 (comment) in review comment #337 (review) (not uncollapsed when using direct link).

anokfireball · 2018-09-13T08:54:08Z

test/SerialIOTest.cpp

 #endif
    REQUIRE(s.getAttribute("vecString").get< std::vector< std::string > >() == std::vector< std::string >({"vector", "of", "strings"}));
-    REQUIRE(s.getAttribute("bool").get< unsigned char >() == static_cast< unsigned char >(true));
+    // REQUIRE(s.getAttribute("bool").get< bool >() == true);


See #337 (comment) in review comment #337 (review) (not uncollapsed when using direct link).

anokfireball

LGTM

ax3l · 2018-09-13T09:01:21Z

@C0nsultant I will rip this PR apart, since it's otherwise too confusing (#333 (comment))

ax3l added bug help wanted tests affects latest release machine/system machine & HPC system specific issues backend labels Sep 6, 2018

ax3l assigned anokfireball Sep 6, 2018

ax3l requested a review from anokfireball September 6, 2018 08:12

ax3l removed the help wanted label Sep 6, 2018

ax3l changed the title ~~Test setAttribute with int/long~~ [WIP] Test setAttribute with int/long Sep 6, 2018

ax3l force-pushed the fix-nonFixedSizeInts branch from d426afc to 7f63290 Compare September 6, 2018 09:04

ax3l force-pushed the fix-nonFixedSizeInts branch from 7f63290 to 115a836 Compare September 6, 2018 09:49

ax3l changed the title ~~[WIP] Test setAttribute with int/long~~ setAttribute with int/long on OSX & Win Sep 6, 2018

ax3l force-pushed the fix-nonFixedSizeInts branch from ceff16b to 8ad30fb Compare September 6, 2018 11:08

anokfireball reviewed Sep 6, 2018

View reviewed changes

ax3l force-pushed the fix-nonFixedSizeInts branch from 8ad30fb to 5000d12 Compare September 6, 2018 11:56

anokfireball reviewed Sep 6, 2018

View reviewed changes

anokfireball approved these changes Sep 6, 2018

View reviewed changes

ax3l mentioned this pull request Sep 6, 2018

Unify Variant.hpp and Attribute.hpp #335

Closed

ax3l commented Sep 6, 2018

View reviewed changes

ax3l force-pushed the fix-nonFixedSizeInts branch from 206e09a to d9b81f7 Compare September 6, 2018 13:35

ax3l commented Sep 6, 2018

View reviewed changes

ax3l changed the title ~~setAttribute with int/long on OSX & Win~~ [WIP] setAttribute with int/long on OSX & Win Sep 7, 2018

ax3l mentioned this pull request Sep 8, 2018

Integer Type System: Refactor #337

Merged

ax3l force-pushed the fix-nonFixedSizeInts branch from d9b81f7 to cb95c26 Compare September 12, 2018 14:09

ax3l commented Sep 12, 2018

View reviewed changes

ax3l added 3 commits September 12, 2018 18:00

Python: Container Namespace Ambiguity

37a6f2e

Fix access to local `detail` namespace.

setAttribute: Explicit C-String Handling

0cc7a84

Explicitly handle C-strings in order to avoid implicit casts. Makes bindings easier.

Tests: setAttribute with int/long

6a77f2e

Test HDF5/ADIOS1 of writing and reading attributes with non-fixed size types.

ax3l force-pushed the fix-nonFixedSizeInts branch from cb95c26 to 6a77f2e Compare September 12, 2018 16:05

anokfireball reviewed Sep 13, 2018

View reviewed changes

anokfireball approved these changes Sep 13, 2018

View reviewed changes

ax3l closed this Sep 13, 2018

This was referenced Sep 13, 2018

setAttribute: Explicit C-String Handling #341

Merged

Tests: setAttribute with int/long #342

Merged

Python: Container Namespace Ambiguity #343

Merged

Attribute: Implement get<>() Cast #345

Merged

ax3l mentioned this pull request Aug 20, 2019

Record_Component.make_empty() #538

Merged

Conversation

ax3l commented Sep 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anokfireball commented Sep 6, 2018

Uh oh!

ax3l commented Sep 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ax3l commented Sep 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ax3l Sep 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ax3l commented Sep 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ax3l commented Sep 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ax3l Sep 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ax3l commented Sep 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ax3l commented Sep 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anokfireball Sep 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anokfireball Sep 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anokfireball left a comment

Choose a reason for hiding this comment

Uh oh!

ax3l commented Sep 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ax3l commented Sep 6, 2018 •

edited

Loading

ax3l Sep 6, 2018 •

edited

Loading

ax3l commented Sep 6, 2018 •

edited

Loading

ax3l commented Sep 6, 2018 •

edited

Loading

ax3l Sep 6, 2018 •

edited

Loading

ax3l commented Sep 6, 2018 •

edited

Loading

anokfireball Sep 13, 2018 •

edited

Loading

anokfireball Sep 13, 2018 •

edited

Loading