Merge lp:~kaaveeacs/drizzle/privatized-members-of-NestedJoin into lp:~drizzle-trunk/drizzle/development

[Kaaveea C S]

[Olaf van der Spek (olafvdspek)]

> List<Item> getSjOuterExprList() const
> void setSjOuterExprList(List<Item> in_sj_outer_expr_list)

Should this be by reference?

Don't forget coding style: first public, than private members.
Also, no need for this:
> join_list(),

review: Needs Fixing

[Monty Taylor (mordred)]

Hi, and thanks for the patch!

There is a random "y" on a line by itself.

there are two getSjCorrTables instead of a getSjCorrTables and a setSjCorrTables

many of the setters should probably be taking their params by const reference

review: Needs Fixing

[Andrew Hutchings (linuxjedi)]

Why is the 'y' on a line on it's own on line 59 of the diff?

[Vijay Samuel (vjsamuel)]

Hi Kaaveea,
  Awesome work on your first branch. Please make sure that your constructor code matches our coding standards. Welcome to Drizzle.

[Olaf van der Spek (olafvdspek)]

Hi Kaaveea ,

On Thu, Apr 14, 2011 at 9:42 PM, Kaaveea C S <email address hidden> wrote:
> +  /*
> +    This constructor serves for creation of NestedJoin instances
> +  */
> +  NestedJoin()
> +  :
> +  join_list(),

Is it necessary to list join_list here?
Is the constructor needed at all?

> +  List<Item> getSjOuterExprList() const

Shouldn't this be returned by const&?

Greetings,

--
Olaf

[Vijay Samuel (vjsamuel)]

Looks good. Awesome job. Got everything right this time. :)

review: Approve

[Olaf van der Spek (olafvdspek)]

See my previous comment.

review: Needs Fixing

[Monty Taylor (mordred)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/14/2011 12:45 PM, Olaf van der Spek wrote:
> Hi Kaaveea ,
>
> On Thu, Apr 14, 2011 at 9:42 PM, Kaaveea C S <email address hidden> wrote:
>> + /*
>> + This constructor serves for creation of NestedJoin instances
>> + */
>> + NestedJoin()
>> + :
>> + join_list(),
>
> Is it necessary to list join_list here?
> Is the constructor needed at all?

Scott Meyers Effective C++ recommendations are to explicitly list all
data members in the initialization list. If you don't list it there,
it's still going to do the constructor, but you may forget that fact.

>> + List<Item> getSjOuterExprList() const
>
> Shouldn't this be returned by const&?

Yes. Yes it should be:

const List<Item>& getSjOuterExprList() const

Monty

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk2ohk8ACgkQ2Jv7/VK1RgEsnwCguHX4lPEdyjhiM5psnmVWRSJK
26kAoMIzM+cJ37vr8h7y0Jhqj+e4mW8r
=iEcT
-----END PGP SIGNATURE-----

[Olaf van der Spek (olafvdspek)]

On Fri, Apr 15, 2011 at 7:59 PM, Monty Taylor <email address hidden> >
Scott Meyers Effective C++ recommendations are to explicitly list all
> data members in the initialization list. If you don't list it there,
> it's still going to do the constructor, but you may forget that fact.

Is that a problem?
--
Olaf

[Monty Taylor (mordred)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/15/2011 11:22 AM, Olaf van der Spek wrote:
> On Fri, Apr 15, 2011 at 7:59 PM, Monty Taylor <email address hidden> >
> Scott Meyers Effective C++ recommendations are to explicitly list all
>> data members in the initialization list. If you don't list it there,
>> it's still going to do the constructor, but you may forget that fact.
>
> Is that a problem?

Don't know if it's purely a problem - that's just the justification of
the rule in general as I understand it. But for the most part, we try to
follow Scott Meyers' suggestions as best as we can. At some point in the
future (once our code is much cleaner) we'd like to be able to turn on
the -Weffc++ warning, which will throw a warning/error on not listing
data members in constructor initialization lists.

Monty
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk2okQYACgkQ2Jv7/VK1RgGPBwCeO1ZEoT6//+uHrXq5+Kdo/OZ3
oqgAn2iT1Lq2ubssdScGH7/OWbp5gFVw
=lSb7
-----END PGP SIGNATURE-----

[Kaaveea C S (kaaveeacs)]

Ive made the changes (retruning a const reference).

Kindly review.

On Fri, Apr 15, 2011 at 10:42 PM, Monty Taylor <email address hidden> wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
>
>
> On 04/15/2011 11:22 AM, Olaf van der Spek wrote:
> > On Fri, Apr 15, 2011 at 7:59 PM, Monty Taylor <email address hidden> >
> > Scott Meyers Effective C++ recommendations are to explicitly list all
> >> data members in the initialization list. If you don't list it there,
> >> it's still going to do the constructor, but you may forget that fact.
> >
> > Is that a problem?
>
> Don't know if it's purely a problem - that's just the justification of
> the rule in general as I understand it. But for the most part, we try to
> follow Scott Meyers' suggestions as best as we can. At some point in the
> future (once our code is much cleaner) we'd like to be able to turn on
> the -Weffc++ warning, which will throw a warning/error on not listing
> data members in constructor initialization lists.
>
> Monty
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.11 (GNU/Linux)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAk2okQYACgkQ2Jv7/VK1RgGPBwCeO1ZEoT6//+uHrXq5+Kdo/OZ3
> oqgAn2iT1Lq2ubssdScGH7/OWbp5gFVw
> =lSb7
> -----END PGP SIGNATURE-----
>
> --
>
> https://code.launchpad.net/~kaaveeacs/drizzle/privatized-members-of-NestedJoin/+merge/57762<https://code.launchpad.net/%7Ekaaveeacs/drizzle/privatized-members-of-NestedJoin/+merge/57762>
> You are the owner of
> lp:~kaaveeacs/drizzle/privatized-members-of-NestedJoin.
>

--
Kaaveea :)

[Vijay Samuel (vjsamuel)]

Third times the charm I guess. Looks good.

review: Approve

[Olaf van der Spek (olafvdspek)]

On Fri, Apr 15, 2011 at 8:42 PM, Monty Taylor <email address hidden> wrote:
>> Scott Meyers Effective C++ recommendations are to explicitly list all
>>> data members in the initialization list. If you don't list it there,
>>> it's still going to do the constructor, but you may forget that fact.
>>
>> Is that a problem?
>
> Don't know if it's purely a problem - that's just the justification of
> the rule in general as I understand it. But for the most part, we try to
> follow Scott Meyers' suggestions as best as we can. At some point in the
> future (once our code is much cleaner) we'd like to be able to turn on
> the -Weffc++ warning, which will throw a warning/error on not listing
> data members in constructor initialization lists.

What concrete advantages will enabling that warning give us?

Olaf

[Monty Taylor (mordred)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/15/2011 04:07 PM, Olaf van der Spek wrote:
> On Fri, Apr 15, 2011 at 8:42 PM, Monty Taylor <email address hidden> wrote:
>>> Scott Meyers Effective C++ recommendations are to explicitly list all
>>>> data members in the initialization list. If you don't list it there,
>>>> it's still going to do the constructor, but you may forget that fact.
>>>
>>> Is that a problem?
>>
>> Don't know if it's purely a problem - that's just the justification of
>> the rule in general as I understand it. But for the most part, we try to
>> follow Scott Meyers' suggestions as best as we can. At some point in the
>> future (once our code is much cleaner) we'd like to be able to turn on
>> the -Weffc++ warning, which will throw a warning/error on not listing
>> data members in constructor initialization lists.
>
> What concrete advantages will enabling that warning give us?

The same advantages that enabling the other warnings we enable gives us
- - a stricter compiler environment, which sometimes helps to find
mistakes at compile time. While there are many times when the warnings
emitted by the compiler can be safely ignored, we have found that a
general policy of keeping the tree warning clean has led to cleaner code
and has actually found bugs before. On the other hand, there is no
particular benefit that we've found to relaxing warnings or ignoring
best practices.

It's one of the things that makes us special and fun!

In the case of this particular warning, if we could get to the point
where we could enable it, warning about missing data members in
initialization list can help to catch things that you forgot to
initialize. It also warns about classes with pointer members which don't
define a copy constructor or an operator= - which is quite dangerous.
Monty
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk2pCF4ACgkQ2Jv7/VK1RgHKUACfWs5CF5PbTkwumOK3Uk78SUmX
StMAnjRsDzP3LjmuRqahyBw2BPtAiXH4
=M2Mc
-----END PGP SIGNATURE-----

[Olaf van der Spek (olafvdspek)]

On Sat, Apr 16, 2011 at 5:13 AM, Monty Taylor <email address hidden> wrote:
> and has actually found bugs before. On the other hand, there is no
> particular benefit that we've found to relaxing warnings or ignoring
> best practices.

In this case I was wondering why those members were being default
initialized. To me, that's unnecessary so I was thinking something
strange was going on.

> It's one of the things that makes us special and fun!
>
> In the case of this particular warning, if we could get to the point
> where we could enable it, warning about missing data members in
> initialization list can help to catch things that you forgot to
> initialize. It also warns about classes with pointer members which don't
> define a copy constructor or an operator= - which is quite dangerous.

Not really, unless it's a ptr to a resource owned by that class.

--
Olaf

[Monty Taylor (mordred)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/18/2011 09:03 AM, Olaf van der Spek wrote:
> On Sat, Apr 16, 2011 at 5:13 AM, Monty Taylor <email address hidden> wrote:
>> and has actually found bugs before. On the other hand, there is no
>> particular benefit that we've found to relaxing warnings or ignoring
>> best practices.
>
> In this case I was wondering why those members were being default
> initialized. To me, that's unnecessary so I was thinking something
> strange was going on.

Unless I'm wrong, I'm pretty sure those members are going to be default
initialized whether they are listed in the initializer list or not. No?

>> It's one of the things that makes us special and fun!
>>
>> In the case of this particular warning, if we could get to the point
>> where we could enable it, warning about missing data members in
>> initialization list can help to catch things that you forgot to
>> initialize. It also warns about classes with pointer members which don't
>> define a copy constructor or an operator= - which is quite dangerous.
>
> Not really, unless it's a ptr to a resource owned by that class.

Yup - but the compiler doesn't know the difference, and it certainly
doesn't hurt to define a copy ctor - so as a rule of thumb, having the
compiler to remind you to define one when you have a pointer data
members doesn't hurt and is a safeguard against a potentially bad mistake.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk2sdJEACgkQ2Jv7/VK1RgFZAACg54ixyvPtSwEJOJ9jPYAK+Es4
dvoAoI8tz9+sYuZTlOSW+hoExW4pqvns
=N+H/
-----END PGP SIGNATURE-----

[Olaf van der Spek (olafvdspek)]

On Mon, Apr 18, 2011 at 7:31 PM, Monty Taylor <email address hidden> wrote:
> Unless I'm wrong, I'm pretty sure those members are going to be default
> initialized whether they are listed in the initializer list or not. No?

Yes. But since those members would normally not be listed, it looks
like something else is happening.

>>> It's one of the things that makes us special and fun!
>>>
>>> In the case of this particular warning, if we could get to the point
>>> where we could enable it, warning about missing data members in
>>> initialization list can help to catch things that you forgot to
>>> initialize. It also warns about classes with pointer members which don't
>>> define a copy constructor or an operator= - which is quite dangerous.
>>
>> Not really, unless it's a ptr to a resource owned by that class.
>
> Yup - but the compiler doesn't know the difference, and it certainly
> doesn't hurt to define a copy ctor - so as a rule of thumb, having the

It does. More code, more complexity, higher chance of bugs. It might
also be slower.

> compiler to remind you to define one when you have a pointer data
> members doesn't hurt and is a safeguard against a potentially bad mistake.

Since such mistakes cause a sure crash the mistake is easily detected anyway.

Olaf

[Stewart Smith (stewart)]

are these called anywhere? I can't see any calling of the new methods. Please use the code somewhere.

review: Needs Fixing

[Vijay Samuel (vjsamuel)]

Hey Stewart,
  This code is not being used anywhere in the code. Thought it would be
something to get your feet wet with as far as revision control and coding
standards is concerned

Cheers,
 -Vijay

On Wed, Apr 20, 2011 at 6:09 AM, Stewart Smith <email address hidden>wrote:

> Review: Needs Fixing
> are these called anywhere? I can't see any calling of the new methods.
> Please use the code somewhere.
> --
>
> https://code.launchpad.net/~kaaveeacs/drizzle/privatized-members-of-NestedJoin/+merge/57940
> You are requested to review the proposed merge of
> lp:~kaaveeacs/drizzle/privatized-members-of-NestedJoin into lp:drizzle.
>

[Olaf van der Spek (olafvdspek)]

On Wed, Apr 20, 2011 at 5:10 PM, Vijay Samuel <email address hidden> wrote:
> Hey Stewart,
>  This code is not being used anywhere in the code. Thought it would be
> something to get your feet wet with as far as revision control and coding
> standards is concerned

That's a shame, as it's likely to be deleted then.

Olaf

=== modified file 'drizzled/nested_join.h'
--- drizzled/nested_join.h	2011-03-28 22:16:45 +0000
+++ drizzled/nested_join.h	2011-04-15 19:08:29 +0000
@@ -30,12 +30,28 @@
 class NestedJoin
 {
 public:
+  /*
+    This constructor serves for creation of NestedJoin instances
+  */
+  NestedJoin() 
+  :
+  join_list(),
+  used_tables(),
+  not_null_tables(),
+  first_nested(NULL),
+  counter_(0),
+  nj_map(),
+  sj_depends_on(),
+  sj_corr_tables(),
+  sj_outer_expr_list()      
+  { }    
+
   /* list of elements in the nested join */
   List<TableList> join_list;
 
   /* bitmap of tables in the nested join */
   table_map used_tables;
-
+  
   /* tables that rejects nulls           */
   table_map not_null_tables;
 
@@ -49,30 +65,71 @@
     by the join optimizer.
     Before each use the counters are zeroed by reset_nj_counters.
   */
+
   uint32_t counter_;
 
   /* Bit used to identify this nested join*/
   std::bitset<64> nj_map;
 
   /*
+     True if this join nest node is completely covered by the query execution
+     plan. This means two things.
+
+     1. All tables on its @c join_list are covered by the plan.
+
+     2. All child join nest nodes are fully covered.
+   */
+
+  bool is_fully_covered() const { return join_list.size() == counter_; }
+
+  /* To get the table_map sj_depends_on */
+  table_map getSjDependsOn() const
+  {
+    return sj_depends_on;
+  }
+
+  /* To set the table_map sj_depends_on */
+  void setSjDependsOn(const table_map &in_sj_depends_on)
+  {
+    sj_depends_on= in_sj_depends_on;
+  }
+
+  /* To get the table_map sj_corr_tables */
+  table_map getSjCorrTables() const
+  {
+    return sj_corr_tables;
+  }
+  
+  /* To set the table_map sj_corr_tables */
+  void setSjCorrTables(const table_map &in_sj_corr_tables) 
+  {
+    sj_corr_tables= in_sj_corr_tables;
+  }
+
+  /* To get the List sj_outer_expr_list */
+  const List<Item>& getSjOuterExprList() const
+  {
+    return sj_outer_expr_list;
+  }
+  
+  /* To set the List sj_outer_expr_list */
+  void setSjOuterExprList(const List<Item> &in_sj_outer_expr_list)
+  {
+    sj_outer_expr_list= in_sj_outer_expr_list;
+  }
+
+private:
+  /*
     (Valid only for semi-join nests) Bitmap of tables outside the semi-join
     that are used within the semi-join's ON condition.
   */
   table_map sj_depends_on;
+  
   /* Outer non-trivially correlated tables */
   table_map sj_corr_tables;
 
   List<Item> sj_outer_expr_list;
 
-  /**
-     True if this join nest node is completely covered by the query execution
-     plan. This means two things.
-
-     1. All tables on its @c join_list are covered by the plan.
-
-     2. All child join nest nodes are fully covered.
-   */
-  bool is_fully_covered() const { return join_list.size() == counter_; }
 };
 
 } /* namespace drizzled */