Merge lp://staging/~bijanbina/ginn/GIR into lp://staging/ginn

I have a number of issues with this proposal.

(1) You need to explicitly justify moving from C to C++. In particular, renaming all existing C sources to C++. What does the project gain from this? What are the possible repercussions (ie. what new problems are introduced and what new potential for error is obtained)? The renaming introduces a lot of noise that makes it difficult to evaluate the rest of the change.

(2) This merge marks most sources as executable. That is undesirable.

(3) What is the .directory file? It does not appear to be necessary to build the project.

(4) Please do not use ///-style or //!-style comments, use Javadoc-style (/**) comments for consistency with the style of other uTouch projects.

(5) The various classes implement their own containers. Is there a good reason not to use the (tested, dependable) standard library instead? One of the main advantages to porting to C++ is the ability to use the C++ library.

(6) None of this code is exception safe. When errors are encountered, resources are leaked or undefined behaviour is encountered.

(7) There is no cleanup: objects are created using new but never deleted.

(8) Class constructors are assigning initial values to members in the constructor body instead of using initializer lists. Most of the classes are C structs that rely on external code to maintain their invariants: what is the point of converting to C++ if the code is just non-OO style C? See http://en.wikipedia.org/wiki/Class_invariant.

(9) Why add a capital G or D to classes and function names? This does not contribute to readability in a positive way. If this is to represent words like "device" or "gesture", use those words. We are not experiencing a shortage of ASCII.

(10) Some variable names could reflect their purpose better: for example, what is "local" used for?

(11) "header.h" is not an acceptable header file name. Also, please do not create a single header file to include all other header files any source file might need. Headers should pull in only the minimal subset of headers they need to compile on their own. Never put a "using namespace" directive in a header file.

(12) GINN::Dadded catches an exception of type error_t, but there is no code inside the try-block that can throw such a type. The only exception possible from the try-block is std::bad_alloc.

(13) Please leave at least one blank line between function definitions for readability.

> I have a number of issues with this proposal.
>
> (1) You need to explicitly justify moving from C to C++. In particular,
> renaming all existing C sources to C++. What does the project gain from this?
> What are the possible repercussions (ie. what new problems are introduced and
> what new potential for error is obtained)? The renaming introduces a lot of
> noise that makes it difficult to evaluate the rest of the change.
>
> (2) This merge marks most sources as executable. That is undesirable.
>
> (3) What is the .directory file? It does not appear to be necessary to build
> the project.
>
> (4) Please do not use ///-style or //!-style comments, use Javadoc-style (/**)
> comments for consistency with the style of other uTouch projects.
>
> (5) The various classes implement their own containers. Is there a good
> reason not to use the (tested, dependable) standard library instead? One of
> the main advantages to porting to C++ is the ability to use the C++ library.
>
> (6) None of this code is exception safe. When errors are encountered,
> resources are leaked or undefined behaviour is encountered.
>
> (7) There is no cleanup: objects are created using new but never deleted.
>
> (8) Class constructors are assigning initial values to members in the
> constructor body instead of using initializer lists. Most of the classes are
> C structs that rely on external code to maintain their invariants: what is
> the point of converting to C++ if the code is just non-OO style C? See
> http://en.wikipedia.org/wiki/Class_invariant.
>
> (9) Why add a capital G or D to classes and function names? This does not
> contribute to readability in a positive way. If this is to represent words
> like "device" or "gesture", use those words. We are not experiencing a
> shortage of ASCII.
>
> (10) Some variable names could reflect their purpose better: for example,
> what is "local" used for?
>
> (11) "header.h" is not an acceptable header file name. Also, please do not
> create a single header file to include all other header files any source file
> might need. Headers should pull in only the minimal subset of headers they
> need to compile on their own. Never put a "using namespace" directive in a
> header file.
>
> (12) GINN::Dadded catches an exception of type error_t, but there is no code
> inside the try-block that can throw such a type. The only exception possible
> from the try-block is std::bad_alloc.
>
> (13) Please leave at least one blank line between function definitions for
> readability.

1: if you think renaming is getting you to trouble so i must do what to port to c++
2: No why you think so
3: i remove it if the code accepted
4: the comment is not part of code! please review the code not comment!
5: can you make instance in my code
6: can you make instance in my code
7: it's the c++ advantage unlike C it delete them automatically
8: it's only on some classes and it is because if we change it need to recreate a code fuly again!!
9: if you use a very long function name you made the developer get to pitfall it's the best way to simple the develop process and have various advantage
10: it use to create a local copy ...

First, Stephen has noted many issues with the changes. I agree with all of them, so it makes sense to have them fixed before doing any further review of the code.

> 1: if you think renaming is getting you to trouble so i must do what to port
> to c++

It's not the renaming that is the problem. If the renaming provided some benefits it would be acceptable. However, the renaming here is to allow for C++ compilation. Thus, there needs to be worthwhile C++ usage or else we could just leave it as C.

This hits at a big issue with the changes. I don't think you fully understand C++, how to use it properly, and how to make code better and more readable with it. Ginn could benefit greatly from being converted to C++. However, these changes do not make the code better.

> 2: No why you think so

See the preview diff below. There are lines like:

=== modified file 'COPYING' (properties changed: -x to +x)

> 3: i remove it if the code accepted

The way to fix this is to remove it in your branch, commit the change, and then push it to this branch on launchpad.net. That will update the merge proposal.

> 4: the comment is not part of code! please review the code not comment!

Comments are just as important as the code. Style matters in keeping things readable. Thus, we review comments too.

> 5: can you make instance in my code
> 6: can you make instance in my code

These are very basic and fundamental concepts of how to write C++. You are asking us to teach you C++. Unfortunately, we don't have enough time to do that. Instead, please try to learn C++ best practices by reading books or searching online for information.

> 7: it's the c++ advantage unlike C it delete them automatically

No, that is incorrect. You must delete C++ objects created with new.

> 8: it's only on some classes and it is because if we change it need to
> recreate a code fuly again!!

This is part of the fundamental aspects of C++. It needs to conform to normal C++ coding standards.

> 9: if you use a very long function name you made the developer get to pitfall
> it's the best way to simple the develop process and have various advantage

There are many possible ways to name functions, with many different reasons for why they are named a certain way. Normally, you should try to stay consistent with the existing code, and ensure that the names are obvious. Adding 'G' and 'D' to the beginning of function names does not have an obvious meaning.

> 10: it use to create a local copy of ginn object that is load for device .it's
> show the duty fully

You can name it "ginn" or something more descriptive. "local" is too generic for a variable name.

> 11: i like this way;also adding header not make your software slow!

You can add a header, that's not the issue. The issue is that the name of the header is too generic. The name needs to give some idea as to what is defined in the header.

> 12: it's an static error ,add error_t is for just through of the compiler
> error. also use error_t for dynamic error.

I don't think you understand how exceptions work. Please look up C++ exception handling.

> 13: the comment is not part of code! please review the code not comment!

This has nothing t...