Merge lp://qastaging/~kaaveeacs/drizzle/privatized-members-of-NestedJoin into lp://qastaging/~drizzle-trunk/drizzle/development
- privatized-members-of-NestedJoin
- Merge into development
Status: | Merged |
---|---|
Approved by: | Brian Aker |
Approved revision: | 2269 |
Merged at revision: | 2290 |
Proposed branch: | lp://qastaging/~kaaveeacs/drizzle/privatized-members-of-NestedJoin |
Merge into: | lp://qastaging/~drizzle-trunk/drizzle/development |
Diff against target: |
114 lines (+67/-10) 1 file modified
drizzled/nested_join.h (+67/-10) |
To merge this branch: | bzr merge lp://qastaging/~kaaveeacs/drizzle/privatized-members-of-NestedJoin |
Related bugs: | |
Related blueprints: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Stewart Smith (community) | Needs Fixing | ||
Olaf van der Spek | Pending | ||
Monty Taylor | Pending | ||
Vijay Samuel | Pending | ||
Review via email: mp+57940@code.qastaging.launchpad.net |
This proposal supersedes a proposal from 2011-04-14.
Commit message
Description of the change
Olaf van der Spek (olafvdspek) wrote : Posted in a previous version of this proposal | # |
Monty Taylor (mordred) wrote : Posted in a previous version of this proposal | # |
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
Andrew Hutchings (linuxjedi) wrote : Posted in a previous version of this proposal | # |
Why is the 'y' on a line on it's own on line 59 of the diff?
Vijay Samuel (vjsamuel) wrote : Posted in a previous version of this proposal | # |
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) wrote : Posted in a previous version of this proposal | # |
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> getSjOuterExprL
Shouldn't this be returned by const&?
Greetings,
--
Olaf
Vijay Samuel (vjsamuel) wrote : Posted in a previous version of this proposal | # |
Looks good. Awesome job. Got everything right this time. :)
Olaf van der Spek (olafvdspek) wrote : Posted in a previous version of this proposal | # |
See my previous comment.
Monty Taylor (mordred) wrote : Posted in a previous version of this proposal | # |
-----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> getSjOuterExprL
>
> Shouldn't this be returned by const&?
Yes. Yes it should be:
const List<Item>& getSjOuterExprL
Monty
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://
iEYEARECAAYFAk2
26kAoMIzM+
=iEcT
-----END PGP SIGNATURE-----
Olaf van der Spek (olafvdspek) wrote : Posted in a previous version of this proposal | # |
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) wrote : Posted in a previous version of this proposal | # |
-----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://
iEYEARECAAYFAk2
oqgAn2iT1Lq2ubs
=lSb7
-----END PGP SIGNATURE-----
Kaaveea C S (kaaveeacs) wrote : Posted in a previous version of this proposal | # |
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://
>
> iEYEARECAAYFAk2
> oqgAn2iT1Lq2ubs
> =lSb7
> -----END PGP SIGNATURE-----
>
> --
>
> https:/
> You are the owner of
> lp:~kaaveeacs/drizzle/privatized-members-of-NestedJoin.
>
--
Kaaveea :)
Vijay Samuel (vjsamuel) wrote : Posted in a previous version of this proposal | # |
Third times the charm I guess. Looks good.
Olaf van der Spek (olafvdspek) wrote : Posted in a previous version of this proposal | # |
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) wrote : Posted in a previous version of this proposal | # |
-----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://
iEYEARECAAYFAk2
StMAnjRsDzP3Ljm
=M2Mc
-----END PGP SIGNATURE-----
Olaf van der Spek (olafvdspek) wrote : Posted in a previous version of this proposal | # |
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) wrote : Posted in a previous version of this proposal | # |
-----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://
iEYEARECAAYFAk2
dvoAoI8tz9+
=N+H/
-----END PGP SIGNATURE-----
Olaf van der Spek (olafvdspek) wrote : Posted in a previous version of this proposal | # |
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) wrote : | # |
are these called anywhere? I can't see any calling of the new methods. Please use the code somewhere.
Vijay Samuel (vjsamuel) 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
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:/
> You are requested to review the proposed merge of
> lp:~kaaveeacs/drizzle/privatized-members-of-NestedJoin into lp:drizzle.
>
Olaf van der Spek (olafvdspek) wrote : | # |
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
> List<Item> getSjOuterExprL ist() const ist(List< Item> in_sj_outer_ expr_list)
> void setSjOuterExprL
Should this be by reference?
Don't forget coding style: first public, than private members.
Also, no need for this:
> join_list(),