Remove FieldTemplate<> from field reflection type #374

veselinp · 2017-03-21T02:10:03Z

FieldTemplate<> params are part of field relfection type, this unnecessary increases generated symbol names.

msftclas · 2017-03-21T02:10:07Z

@veselinp,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

chwarr

The Clang build is failing with this change. Can you look into what's wrong?

chwarr · 2017-03-21T05:57:30Z

CHANGELOG.md

+### C++ & `gbc` ###
+
+* Remove FieldTemplate details from field reflection type.
+


Minor: there's a gbc section right below where this would make more sense. Consider also duplicating the entry in the C++ section as well.

chwarr · 2017-03-21T05:57:51Z

CHANGELOG.md

@@ -19,6 +19,10 @@ different versioning scheme, following the Haskell community's
 * C# NuGet version: TBD (minor bump needed)
 * C# Comm NuGet version: TBD (minor bump needed)

+### C++ & `gbc` ###
+
+* Remove FieldTemplate details from field reflection type.


Minor: can you expand on this and say why we're making the change (to shorten the symbol names)?

veselinp · 2017-03-24T06:05:20Z

I've pushed a fixup, clang should be happy now.

chadwalters · 2017-03-28T17:42:34Z

clang build is happy. Looks like VC12 is still broken though.

veselinp · 2017-03-28T22:21:19Z

vc12 doesn't understand constexpr :)

veselinp · 2017-03-29T04:06:31Z

now, clang is broken, I'm looking again

chadwalters · 2017-03-30T22:38:34Z

cpp/test/core/container_extensibility.cpp

-                    ordered_non_unique_field<SimpleStructView::Schema::var::m_str>,
-                    ordered_non_unique_field<SimpleStructView::Schema::var::m_uint64>
+                    ordered_non_unique_field<SimpleStructView::Schema::var::m_str, get_field_template<SimpleStructView::Schema::var::m_str>::type>,
+                    ordered_non_unique_field<SimpleStructView::Schema::var::m_uint64, get_field_template<SimpleStructView::Schema::var::m_uint64>::type>


Can you explain this part of the change?

I simplified this, and moved all logic into ordered_non_unique_field<>, please check cpp/test/core/multi_index.h

vc12 doesn't support constexp - ordered_non_unique_field requires member field address in compile time, which can be resolved via FieldTemplate.

chadwalters · 2017-03-30T22:40:13Z

CHANGELOG.md

@@ -21,6 +21,9 @@ different versioning scheme, following the Haskell community's

 ### `gbc` and Bond compiler library ###

+* C++ codegen generates field reflection, that uses an explicit type 
+  derrived from reflection::FieldTemplate. This hides FieldTemplate 
+  details and, as result, shortens symbol names.


Minor: Change to "C++ codegen hides FieldTemplate details, shortening symbol names."

chwarr · 2017-04-10T19:03:40Z

I will look again later today.

chwarr · 2017-04-17T20:31:37Z

cpp/inc/bond/core/reflection.h

@@ -101,24 +101,21 @@ struct FieldTemplate
    /// @brief Static data member describing field metadata
    static const Metadata& metadata;

-    /// @brief Static data member representing the field pointer
-    static const field_pointer field;
-


This looks like a breaking change (removal of public member of public interface). Is it intentional? If so, what's motivating making this breaking change?

It appears that the replacement for this functionality is &theFieldType.GetVariable(someStruct).

Good point, let me revisit this.

If this materially reduces the lib and PDB size, then it's something we can consider. The next release for C++ will have some other breaking changes (albeit minor ones) like this.

I added it back for now, we may want to handle vc12 explicitly to remove this symbol from generated .obj files.

chwarr · 2017-04-17T20:34:52Z

python/inc/bond/python/struct.h

@@ -26,14 +26,14 @@ class def_readwrite_property
        : c(c)
    {}

-    template <typename Field>
-    void operator()(const Field&) const
+    template <uint16_t field_id, typename ModifierTag, typename Struct, typename FieldType, FieldType Struct::*field_ptr, const bond::Metadata* metadata_ptr>


Was this change required to get this to build?

If so, then the introduction of the anonymous struct is not working as I'd expect it to (the anonymous struct should have an is a relationship with FieldTemplate), and is a large breaking change.

If this was just done to clean up the code, that's fine, but I need to get confirmation from you.

This change is related to FieldTemplate<>::field change. I will revert this file for now.

- FieldTemplate<> params are part of field reflection type, this unnecessary increases size of generated symbol names.

chwarr

I'll merge this in now. By the way, I needed to fix up the changelog section. That merge didn't go well.

CI failure is MSVC 2013 timeout...

msftclas added the cla-not-required label Mar 21, 2017

chwarr requested changes Mar 21, 2017

View reviewed changes

chadwalters reviewed Mar 30, 2017

View reviewed changes

chwarr requested changes Apr 17, 2017

View reviewed changes

Veselin Petrov added 8 commits April 19, 2017 17:52

[C++] Remove FieldTemplate<> from field reflection

9de48b7

- FieldTemplate<> params are part of field reflection type, this unnecessary increases size of generated symbol names.

fixup! [C++] Remove FieldTemplate<> from field reflection

8eb7ee7

fixup! [C++] Remove FieldTemplate<> from field reflection

8a8fabf

fixup! [C++] Remove FieldTemplate<> from field reflection

14232c3

fixup! [C++] Remove FieldTemplate<> from field reflection

3e4dda4

fixup! [C++] Remove FieldTemplate<> from field reflection

3f6c63c

fixup! [C++] Remove FieldTemplate<> from field reflection

c7051a0

fixup! [C++] Remove FieldTemplate<> from field reflection

d1a1728

chwarr approved these changes Apr 25, 2017

View reviewed changes

chwarr closed this in 906272b Apr 25, 2017

veselinp deleted the hide_field_template_details_from_reflection branch May 1, 2017 23:31

chwarr mentioned this pull request Mar 14, 2020

Bond build failed with error C5208 in MSVC #1027

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove FieldTemplate<> from field reflection type #374

Remove FieldTemplate<> from field reflection type #374

veselinp commented Mar 21, 2017

msftclas commented Mar 21, 2017

chwarr left a comment

chwarr Mar 21, 2017

veselinp Mar 28, 2017

chwarr Mar 21, 2017

veselinp Mar 28, 2017

veselinp commented Mar 24, 2017

chadwalters commented Mar 28, 2017

veselinp commented Mar 28, 2017

veselinp commented Mar 29, 2017

chadwalters Mar 30, 2017

veselinp Apr 3, 2017

chadwalters Mar 30, 2017

veselinp Apr 3, 2017

chwarr commented Apr 10, 2017

chwarr Apr 17, 2017

veselinp Apr 19, 2017

chwarr Apr 19, 2017

veselinp Apr 24, 2017

chwarr Apr 17, 2017

veselinp Apr 20, 2017 •

edited

Loading

chwarr left a comment •

edited

Loading

		### C++ & `gbc` ###

		* Remove FieldTemplate details from field reflection type.

Remove FieldTemplate<> from field reflection type #374

Remove FieldTemplate<> from field reflection type #374

Conversation

veselinp commented Mar 21, 2017

msftclas commented Mar 21, 2017

chwarr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veselinp commented Mar 24, 2017

chadwalters commented Mar 28, 2017

veselinp commented Mar 28, 2017

veselinp commented Mar 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chwarr commented Apr 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veselinp Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

chwarr left a comment • edited Loading

Choose a reason for hiding this comment

veselinp Apr 20, 2017 •

edited

Loading

chwarr left a comment •

edited

Loading