[GenABEL-dev] [Genabel-commits] r1837 - in pkg/OmicABELnoMM: . examples src tests

[Frank, Alvaro Jesus]

Hi Lennart,

Sorry for the misunderstanding regarding vector. I really rather keep the arrays though.
I will try to break long lines on my next code cleanup pass and do an A-style auto formatting.
I am focused on the documentation to make sure users can get started. I will try to sit with Yakov and Sodbo to show them the program and its usage (they are in Munich).  

Alvaro
________________________________________
From: L.C. Karssen [l.c.karssen at polyomica.com]
Sent: Thursday, October 23, 2014 6:28 PM
To: Frank, Alvaro Jesus; genabel-devel at lists.r-forge.r-project.org
Subject: Re: [GenABEL-dev] [Genabel-commits] r1837 - in pkg/OmicABELnoMM: . examples src tests

Hi Alvaro,

On 21-10-14 18:47, Frank, Alvaro Jesus wrote:
>
>     The if statements below are a pain to my eyes :-). Due to the formatting
>     (missing indentation and/or missing curly braces) it gives the
>     impression that both the cout and the i = statement are carried out if
>     the condition is true (or maybe even that nothing happens in the
>     if-statement if you take the lack of indentation seriously).
>
>     At the very least I would indent the cout statement, but given that it
>     is a long line anyway (which are best avoided), I would add {}s and
>     break the cout statement over multiple lines.
>
>
> I will do a code cleanup and address this issues.
>

That'd be great. Thanks!

>
>
>     > + if(params.mpi_id == 0)
>     > cout << "Warning, ID names between -g file and interactions file
>     do not coincide! The files must have the same meaningful order!" <<
>     endl;
>     > i = params.n;//exit
>     > }
>     > @@ -433,7 +461,7 @@
>     > }
>     > }
>     > }
>     > - if(interaction_missings)
>
>     This if statement is indented as it should be, although I would prefer
>     breaking the long line.
>
> Editor and screen size dependent, but I can comply.

True, but the fact that it's screen size dependent is one of the reasons
to stick to a standard width.


>
>
>     Have you ever considered using C++ vectors instead of arrays? They save
>     you the trouble of new/delete, for example. I have no idea if there is a
>     performance hit, however. But I can imagine that combined with iterators
>     they may be well-optimised.
>     Or do you simply use them because LAPACK needs arrays anyway?
>

If I understand your answer below correctly, it looks like you
misunderstood what I tried to say. So, before we get into technical
stuff: in this case I was mostly wondering about whether using
std::vector would be slower (or faster) than the current use of arrays
in the case of OmicABELnoMM. See for example
http://lemire.me/blog/archives/2012/06/20/do-not-waste-time-with-stl-vectors/.

And the last part of my question was: maybe using std::vectors is simply
not even possible because you need to pass them on to LAPACK routines,
which accept pointers to arrays, IIRC.

Now going back to what you wrote:

>
> ISO C++ forbids variable-size array. I cannot comply with every style
> and avoid all possible warnings at the same time.


I don't think I understand your first point. Variable-length arrays
(VLAs) are indeed not ISO C++ (yet), but that's not what I was after. I
was wondering if std::vector would be (a lot) slower than your current
solution. In terms of 'bookkeeping', STL vectors are definitely easier,
which is why I prefer to use them. I just don't know if they are much
slower in a program as optimised as OmicABELnoMM.

About the 'bookkeeping':
Currently you use new to allocate memory on the heap and delete[] for
deallocation. Using std::vector removes that need. Moreover, if you use
new to allocate arrays you should actually check whether the allocation
succeeded.
As an example of how easy it is to make a mistake, check line 1908 of
src/AIOwrapper.cpp, where you use a delete that should have been a
delete[] (thanks to Jenkins' cppcheck for pointing this one out).

As for the C++ standard to use (if that is what you mean with 'style'),
currently you have -std=c++11 in the default CXXFLAGs in configure.ac.
I'd stick with that.

About avoiding warnings: Creating warning-free code is paramount IMO,
because it leads to code that is less prone to bugs and is easier to
maintain.



>
>     > Modified: pkg/OmicABELnoMM/src/main.cpp
>     > ===================================================================
>     > --- pkg/OmicABELnoMM/src/main.cpp 2014-10-17 14:57:34 UTC (rev 1836)
>     > +++ pkg/OmicABELnoMM/src/main.cpp 2014-10-21 13:30:17 UTC (rev 1837)
>     > @@ -18,39 +18,41 @@
>     > -d <0.0~1.0> -r <-10.0~1.0> -b -s <0.0~1.0> -e <-10.0~1.0> -i -f";
>     >
>     > string helpcmd_expl =
>
>
>     Note that you can use the information generated by autotools for version
>     info. If you include the config.h file that is generated you can do it
>     like we did in ProbABEL, for example in src/usage.cpp:
>
>     void print_version(void) {
>     cout << PACKAGE
>     << " v. " << PACKAGE_VERSION
>     << "\n(C) Yurii Aulchenko, Lennart C. Karssen, Maarten Kooyman, "
>     << "Maksim Struchalin, The GenABEL team, EMC Rotterdam\n\n";
>     cout << "Using EIGEN version " << EIGEN_WORLD_VERSION
>     << "." << EIGEN_MAJOR_VERSION << "." << EIGEN_MINOR_VERSION
>     << " for matrix operations\n";
>     }
>
>     The PACKAGE and PACKAGE_VERSION variables comes from config.h, and are
>     generated by autotools. OmicABELnoMM's src/config.h contains them as
>     well.
>
>
> I will try it
>



Let me know if it doesn't work out.


Best,

Lennart.

--
*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*
L.C. Karssen
Utrecht
The Netherlands

lennart at karssen.org
http://blog.karssen.org
GPG key ID: A88F554A
-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-*-