Check variadic pattern arguments at compile time #356

devreal · 2017-04-03T15:51:36Z

This PR changes dash::internal::PatternArguments such that variadic pattern argument integrity is checked at compile-time (except for tile-correctness). It also enables arbitrary mixing of SizeType, Distribution, and Team|TeamSpec arguments, as long as the argument list starts with a SizeType. Most notably, it catches mixed usage of Team and TeamSpec, Distribution and DistributionSpec, as well as SizeType and SizeSpec specifications at compile-time.

My attempt to add some constexpr magic (as proposed in #354) failed since this style of parsing does not seem to be possible that way as it would require overloading constructor templates.

As an addition, this PR also introduces DASH_ASSERT_ALWAYS, which always asserts even if assertions are disabled. This is required to ensure pattern validity (replaced plain assert).

… mixed size/distribution specification)

ddiefenthaler

Looks good, I'll leave some comments inline

ddiefenthaler · 2017-04-04T01:27:25Z

dash/include/dash/pattern/internal/PatternArguments.h

@@ -122,71 +105,91 @@ class PatternArguments {

 private:
  /// BlockPattern matching for extent value of type IndexType.
-  template<int count>
-  void check(SizeType extent) {
+  template<int argc_size, int argc_dist, int has_team, typename ... Args>


Some documentation would be nice for those parameters.

ddiefenthaler · 2017-04-04T01:43:41Z

dash/include/dash/pattern/internal/PatternArguments.h

    _teamspec   = teamSpec;
+    check<argc_size, argc_dist, -1>(std::forward<Args>(args)...);


No Team is set in this case, it was probably broken before as well...

ddiefenthaler · 2017-04-04T01:44:32Z

dash/include/dash/pattern/internal/PatternArguments.h

-      _teamspec = TeamSpec_t(_distspec, team);
-    }
+    _team     = &team;
+    check<argc_size, argc_dist, 1>(std::forward<Args>(args)...);


No TeamSpec is created in this case,
as it is dependend on _distspec it probably has to go in the final check() call.

ddiefenthaler · 2017-04-04T01:48:29Z

dash/include/dash/pattern/internal/PatternArguments.h

-  template<int count>
-  void check(const SizeSpec_t & sizeSpec) {
+  template<int argc_size, int argc_dist, int has_team, typename ... Args>
+  void check(const SizeSpec_t & sizeSpec, Args && ... args) {


Perhaps it makes sense to move this function up,
so that the it is grouped with the function taking individual extents.

ddiefenthaler · 2017-04-04T01:59:28Z

dash/include/dash/pattern/internal/PatternArguments.h

-    _argc_size++;
-    _sizespec.resize(count, extent);
+    _sizespec.resize(argc_size, extent);
+    check<argc_size+1, argc_dist, has_team>(std::forward<Args>(args)...);
  }
  /// BlockPattern matching for up to \c NumDimensions optional 
  /// parameters specifying the distribution pattern.


This comment seems to be shifted.

ddiefenthaler · 2017-04-04T02:01:29Z

dash/test/pattern/BlockPatternTest.cc

@@ -18,14 +18,16 @@ TEST_F(BlockPatternTest, SimpleConstructor)
  int extent_x = 21;
  int extent_y = 37;
  int extent_z = 41;
-  int size = extent_x * extent_y * extent_z;
+  int size = extent_x * extent_x * extent_z;


This change was probably uninteded, as it is currently breaking the tests.

ddiefenthaler · 2017-04-04T02:04:02Z

dash/test/pattern/BlockPatternTest.cc

+      dash::Team::All());
+  EXPECT_EQ(ds_blocked_z, pat_ds_t.distspec());
+  EXPECT_EQ(size, pat_ds_t.capacity());
+  EXPECT_EQ(dash::Team::All().size(), pat_ds_t.num_units());


The wrong patterns are checked in those EXPECTs.

ddiefenthaler · 2017-04-04T02:04:38Z

dash/test/pattern/BlockPatternTest.cc

  // Should default to distribution BLOCKED, NONE, NONE:
  dash::BlockPattern<3> pat_default(extent_x, extent_y, extent_z);
  EXPECT_EQ(dash::DistributionSpec<3>(), pat_default.distspec());
  EXPECT_EQ(dash::Team::All(), pat_default.team());
  EXPECT_EQ(dash::Team::All().size(), pat_default.num_units());
  EXPECT_EQ(size, pat_default.capacity());

+  dash::SizeSpec<3> sspec(
+      extent_x, extent_x, extent_z);


Probably the wrong y-extent here, too.

ddiefenthaler · 2017-04-04T02:07:25Z

dash/include/dash/pattern/internal/PatternArguments.h

+  void check(const TeamSpec_t & teamSpec, Args && ... args) {
+    static_assert(has_team == 0,
+        "Cannot mix Team and TeamSpec definition in variadic "
+        "pattern constructor!");


This limitation seems a little bit odd,
but I think this is depended on TeamSpec.

ddiefenthaler · 2017-04-04T02:14:00Z

dash/include/dash/pattern/internal/PatternArguments.h

-        "expected " << NumDimensions << ", got " << _argc_dist);
-    }
-    check_tile_constraints();
+    check<0, 0, 0>(std::forward<Args>(args)...);


Meanwhile it is probably more a parse than a check.

ddiefenthaler

It looks good now and should be ready to merge.

I found one more minor thing that is TeamSpec related,
but I think TeamSpec needs its own issue/pull request, so it shouldn't be important for now.

ddiefenthaler · 2017-04-09T11:36:41Z

dash/include/dash/pattern/internal/PatternArguments.h

-      _teamspec = TeamSpec_t(_distspec, team);
-    }
+    _team     = &team;
+    _teamspec = TeamSpec_t(team);


Possibly overwrites previously set TeamSpec.
Should already be handled by check in line 255.

ddiefenthaler · 2017-04-09T11:40:53Z

dash/include/dash/pattern/internal/PatternArguments.h

+     */
+    if (ArgcTeam && !ArgcTeamSpec) {
+      _teamspec = TeamSpec_t(_distspec, *_team);
+    }


I think it is possible to return without _team set, how about:

if(!ArgcTeam) { _team = &dash::Team::All() } if(!ArgcTeamSpec) { _teamspec = TeamSpec_t(_distspec, *_team); }

I'm not sure about the use cases of this constructor,
so perhaps it is also fine to return without a team.

The dash::Team::All is used by default if no team has been set, see team(). The assumption here is that it only makes sense to overwrite the default TeamSpec if a team was set. However, I realized that I am not sure about the relation between DistSpec and TeamSpec and whether we have to create the latter if the former was explicitely given bu the user. Is it even valid to specify both at the same time? @fuchsto can you shed some light?

ddiefenthaler · 2017-04-12T16:04:16Z

dash/include/dash/pattern/internal/PatternArguments.h

-    _distspec   = ds;
+    _distspec = ds;
+    parse<ArgcSize, -1, ArgcTeam,
+      ArgcTeam>(std::forward<Args>(args)...);


Is the use of ArgcTeam instead of ArgcTeamSpec intended here?

ddiefenthaler · 2017-04-12T16:04:24Z

dash/include/dash/pattern/internal/PatternArguments.h

-    }
+    _distspec[ArgcDist] = ds;
+    parse<ArgcSize, ArgcDist+1, ArgcTeam,
+      ArgcTeam>(std::forward<Args>(args)...);


fuchsto · 2017-04-12T05:01:14Z

dash/include/dash/pattern/internal/PatternArguments.h

@@ -25,7 +27,7 @@ template<
  dim_t NumDimensions,
  typename IndexType = dash::default_index_t>
 class PatternArguments {
-private:
+public:


Why public?

devreal · 2017-04-18T08:20:03Z

Thanks for the reviews, they should be addressed.

devreal added 2 commits April 3, 2017 17:31

Extend compile-time argument checking in PatternArguments (now allows…

d7dd350

… mixed size/distribution specification)

Introduce DASH_ASSERT_ALWAYS and always assert pattern correctness

1848471

devreal added the enhancement label Apr 3, 2017

devreal added this to the dash-0.3.0 milestone Apr 3, 2017

devreal assigned fuchsto, devreal and ddiefenthaler Apr 3, 2017

devreal requested a review from fuchsto April 3, 2017 15:51

Check for upper bounds in PatternArguments

59769a3

ddiefenthaler reviewed Apr 4, 2017

View reviewed changes

devreal added 2 commits April 5, 2017 16:07

Fix comments from Daniel

782ddbf

Fix naming and creation of TeamSpec in PatternArguments

1b03058

ddiefenthaler approved these changes Apr 9, 2017

View reviewed changes

devreal and others added 2 commits April 10, 2017 13:04

Avoid overwriting TeamSpec in PatternArguments

2fe333c

Merge branch 'development' into dash-patternargs-comptime

be33b8d

ddiefenthaler reviewed Apr 12, 2017

View reviewed changes

fuchsto requested changes Apr 12, 2017

View reviewed changes

Make typedefs private and fix template arguments in PatternArguments

dfe32d8

Merge branch 'development' into dash-patternargs-comptime

471bb0e

Merge branch 'development' into dash-patternargs-comptime

3583d18

fuchsto approved these changes May 30, 2017

View reviewed changes

fuchsto merged commit 59238d3 into development May 30, 2017

fuchsto deleted the dash-patternargs-comptime branch May 30, 2017 01:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check variadic pattern arguments at compile time #356

Check variadic pattern arguments at compile time #356

devreal commented Apr 3, 2017

ddiefenthaler left a comment

ddiefenthaler Apr 4, 2017

ddiefenthaler Apr 4, 2017

ddiefenthaler Apr 4, 2017

ddiefenthaler Apr 4, 2017

ddiefenthaler Apr 4, 2017

ddiefenthaler Apr 4, 2017

ddiefenthaler Apr 4, 2017

ddiefenthaler Apr 4, 2017

ddiefenthaler Apr 4, 2017

ddiefenthaler Apr 4, 2017

ddiefenthaler left a comment

ddiefenthaler Apr 9, 2017

ddiefenthaler Apr 9, 2017

devreal Apr 19, 2017

ddiefenthaler Apr 12, 2017

ddiefenthaler Apr 12, 2017

fuchsto Apr 12, 2017

devreal commented Apr 18, 2017

		_teamspec = teamSpec;
		check<argc_size, argc_dist, -1>(std::forward<Args>(args)...);

Check variadic pattern arguments at compile time #356

Check variadic pattern arguments at compile time #356

Conversation

devreal commented Apr 3, 2017

ddiefenthaler 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

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

Choose a reason for hiding this comment

ddiefenthaler 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devreal commented Apr 18, 2017