Mark OpenMS::String as deprecated. [448]

[timosachsenberg]

Submitted by witek96 on 2012-08-20 11:30:17

Although OpenMS::String is WRONGLY designed (inherits from std::string) I would not waste time anymore to fix it just mark it as deprecated.

What are the requirements justifying having this class? Can't see anything this class implements which can't be accomplished easily and better with std::string and boost::lexical_cast. This class increases the size of OpenMS API while not adding any value.

Why wrongly designed? DON'T DERIVE FROM STRING!

std::string, that is, basically the whole basic_string template is not designed to be derived from. There are zillions of articles about that already. It doesn't have any virtual functions so there is nothing to override. The best you can do is hide something. Best is to use composition/aggregation! That is, just keep a member of type string in your class and forward the calls! Again, just to make sure

for more information:

http://stackoverflow.com/questions/4205050/inheriting-and-overriding-functions-of-a-stdstring

---

Commented by hendrikweisser on 2012-08-20 16:20:42: There's a large number of utility methods in String that we need. There are probably equivalent methods in Qt's QString, so I'd rather use that as an alternative.

I don't know what Chris meant with the following, though (in his answer to ticket #447):

Our current chaining-capabilities of String are pretty powerful, and Qt for example doesn't have it.

---

Commented by aiche on 2012-08-20 17:13:50: While I'm not sure if marking it as deprecated is the correct solution I agree with the general concern of wrong design. I think this falls into one category with MSExperiment, MSSpectrum, and friends who are all derived from std::vector.

I added the String class to the Wiki page for the discussion on [wiki:OpenMS2.0Planing OpenMS 2.0].

[hendrikweisser]

Are the any plans to actually go through with this (deprecating)? If not, should we close the ticket?

[timosachsenberg]

I would like to keep it open and throw in some material: e.g. I like the approach of POCO: https://pocoproject.org/slides/040-StringsAndFormatting.pdf which is pretty close to what @hroest did with the StringUtils class. What I like about the POCO approach is that they provide both in place and copy version of the string manipulation functions.

[hendrikweisser]

I like the approach of POCO

Looks nice. Is that a library that we could include? I think there are too many string operations that we've implemented ourselves (some of them probably badly), and I would prefer it if we could use e.g. the QString (or POCO) versions instead.

[timosachsenberg]

I think it might be worth to take a look at it or e.g. copy party of POCO into our StringUtils class. I would also prefer not to use QString as a default as this means we will never get rid of the Qt dependency in the core part of OpenMS (which in my view would be a good thing if we want to modularize OpenMS a bit more in the future). Most core functionality rely on standard string functionality (+ some boost stuff). AFAIK QString is mainly used in the non-core parts that rely on other Qt libraries (e.g. that require a GUI or network)

[hroest]

:+1: for keeping Qt as much as possible out of core OpenMS

I just did a quick analysis of all the includes

grep -R '#include'  src/openms/   | grep -v '<OpenMS' | cut -f 2 -d '#' | sort | uniq -c  | sort -n  > /tmp/all_includes
grep -R '#include'  src/openms/   | grep -v '<OpenMS' | cut -f 2 -d '#' | sort | uniq -c | sort -n | grep -v 'hpp>'  | grep -v '\.h>' | grep -v '\.h"' | grep -v '<Qt' | grep -v Eigen > /tmp/std_includes

$ diff /tmp/all_includes  /tmp/std_includes 35,67d34
<       1 include <boost/accumulators/statistics/covariance.hpp>
<       1 include <boost/accumulators/statistics/variates/covariate.hpp>
<       1 include <boost/algorithm/string.hpp>
<       1 include <boost/algorithm/string/replace.hpp>
<       1 include <boost/algorithm/string/split.hpp>
<       1 include <boost/algorithm/string/trim.hpp>
<       1 include <boost/array.hpp>
<       1 include <boost/assign/list_of.hpp>
<       1 include <boost/bimap.hpp>
<       1 include <boost/bimap/multiset_of.hpp>
<       1 include <boost/cast.hpp>
<       1 include <boost/date_time/posix_time/posix_time_types.hpp> //no i/o just types
<       1 include <boost/foreach.hpp>
<       1 include <boost/functional/hash.hpp>
<       1 include <boost/function/function_base.hpp>
<       1 include <boost/interprocess/sync/file_lock.hpp>
<       1 include <boost/lambda/casts.hpp>
<       1 include <boost/lambda/lambda.hpp>
<       1 include <boost/math/distributions/cauchy.hpp>
<       1 include <boost/math/special_functions/bessel.hpp>
<       1 include <boost/math/special_functions/digamma.hpp>
<       1 include <boost/math/special_functions/factorials.hpp>
<       1 include <boost/math/special_functions/fpclassify.hpp> // boost::math::isfinite
<       1 include <boost/mem_fn.hpp>
<       1 include <boost/numeric/conversion/cast.hpp>
<       1 include <boost/random/binomial_distribution.hpp>
<       1 include <boost/random/cauchy_distribution.hpp>
<       1 include <boost/random/exponential_distribution.hpp>
<       1 include <boost/random/poisson_distribution.hpp>
<       1 include <boost/range/algorithm/copy.hpp>
<       1 include <boost/ref.hpp>
<       1 include <boost/spirit/include/qi.hpp>
<       1 include <boost/unordered/unordered_map.hpp>
69d35
<       1 include "BSplineBase.h"
73d38
<       1 include <bzlib.h>
97d61
<       1 include <CrawdadWrapper.h>
105,109d68
<       1 include <eigen3/unsupported/Eigen/NonLinearOptimization>
<       1 include <Eigen/Dense>
<       1 include <Eigen/SVD>
<       1 include <errno.h>
<       1 include "f2c.h" -- removed */
111d69
<       1 include <glpk.h>
122,124d79
<       1 include <mach/mach.h>
<       1 include <mach/mach_init.h>
<       1 include <mach-o/dyld.h>
129,132d83
<       1 include "OpenMS/ANALYSIS/OPENSWATH/OPENSWATHALGO/ALGO/Scoring.h"
<       1 include "OpenMS/MATH/STATISTICS/StatisticFunctions.h"
<       1 include <process.h>
<       1 include "psapi.h"
135d85
<       1 include <QCoreApplication.h> // needed to disable plugin loading on Mac OSX
139,143d88
<       1 include <QtCore/QDateTime>
<       1 include <QtCore/QFileInfo>
<       1 include <QtCore/QFileSystemWatcher>
<       1 include <QtCore/QList>
<       1 include <QtCore/QUrl>
145,155d89
<       1 include <QtNetwork/QHostInfo>
<       1 include <QtNetwork/QHttpRequestHeader>
<       1 include <QtNetwork/QNetworkReply>
<       1 include <seqan/align.h>
<       1 include <seqan/basic.h>
<       1 include <seqan/graph_align.h>
<       1 include <seqan/index.h>
<       1 include <seqan/seq_io/guess_stream_format.h>
<       1 include <seqan/seq_io/read_fasta_fastq.h>
<       1 include <seqan/sequence.h>
<       1 include <seqan/stream.h>
159,165d92
<       1 include <stdint.h>
<       1 include <sys/timeb.h>
<       1 include <sys/times.h>
<       1 include <sys/utime.h>
<       1 include <time.h>
<       1 include <unistd.h> // for getpid
<       1 include <unsupported/Eigen/Splines>
171,196d97
<       1 include <windows.h>
<       1 include "windows.h"
<       1 include <windows.h> // for GetConsoleScreenBufferInfo()
<       1 include <Wm5ApprLineFit2.h>
<       1 include "Wm5ApprLineFit2.h"
<       1 include <Wm5IntpAkimaNonuniform1.h>
<       1 include <Wm5Math.h>
<       1 include <Wm5Vector2.h>
<       1 include <xercesc/dom/DOMDocument.hpp>
<       1 include <xercesc/dom/DOMDocumentType.hpp>
<       1 include <xercesc/dom/DOM.hpp>
<       1 include <xercesc/dom/DOMImplementation.hpp>
<       1 include <xercesc/dom/DOMImplementationLS.hpp>
<       1 include <xercesc/dom/DOMNodeIterator.hpp>
<       1 include <xercesc/framework/LocalFileFormatTarget.hpp>
<       1 include <xercesc//framework/psvi/XSValue.hpp>
<       1 include <xercesc/framework/XMLFormatter.hpp>
<       1 include <xercesc/internal/MemoryManagerImpl.hpp>
<       1 include <xercesc/sax2/DefaultHandler.hpp>
<       1 include <xercesc/sax/ErrorHandler.hpp>
<       1 include <xercesc/sax/InputSource.hpp>
<       1 include <xercesc/sax/Locator.hpp>
<       1 include <xercesc/util/OutOfMemoryException.hpp>
<       1 include <xercesc/util/XMLUniDefs.hpp>
<       1 include <xercesc/util/XMLUni.hpp>
<       1 include <xercesc/validators/common/Grammar.hpp>
198,213d98
<       2 include "BandedMatrix.h"
<       2 include <boost/accumulators/accumulators.hpp>
<       2 include <boost/accumulators/statistics/mean.hpp>
<       2 include <boost/accumulators/statistics/stats.hpp>
<       2 include <boost/accumulators/statistics/variance.hpp>
<       2 include <boost/assign.hpp>
<       2 include <boost/iterator/indirect_iterator.hpp> // for equality
<       2 include <boost/make_shared.hpp>
<       2 include <boost/math/special_functions/binomial.hpp>
<       2 include <boost/math/special_functions/fpclassify.hpp> // for "isnan"
<       2 include <boost/math/tr1.hpp>
<       2 include <boost/numeric/conversion/cast.hpp>
<       2 include <boost/random/discrete_distribution.hpp>
<       2 include <boost/random/normal_distribution.hpp>
<       2 include <boost/range/adaptor/map.hpp>
<       2 include "BSpline.h"
216,217d100
<       2 include <eigen3/Eigen/Core>
<       2 include <Eigen/LU>
222,230d104
<       2 include <QtCore/QDate>
<       2 include <QtCore/QObject>
<       2 include <QtCore/QProcess>
<       2 include <QtCore/QRegExp>
<       2 include <QtGui/QTextDocument>
<       2 include <QtNetwork/QNetworkRequest>
<       2 include <sys/stat.h>
<       2 include <sys/time.h>
<       2 include <unistd.h>
232,245d105
<       2 include <utime.h>
<       2 include "Wm5LinearSystem.h"
<       2 include <xercesc/dom/DOMText.hpp>
<       2 include <xercesc/framework/LocalFileInputSource.hpp>
<       2 include <xercesc/framework/MemBufInputSource.hpp>
<       2 include <xercesc/sax2/SAX2XMLReader.hpp>
<       2 include <xercesc/sax2/XMLReaderFactory.hpp>
<       2 include <xercesc/util/BinInputStream.hpp>
<       2 include <zlib.h>
<       3 include <boost/math/special_functions/acosh.hpp>
<       3 include <boost/math/special_functions/erf.hpp>
<       3 include <boost/math/special_functions/fpclassify.hpp> // for isnan
<       3 include <boost/math/special_functions/gamma.hpp>
<       3 include <boost/random/uniform_real.hpp>
250,252d109
<       3 include <QtCore/QDir>
<       3 include <QtCore/QStringList>
<       3 include <QtCore/QTimer>
254,255d110
<       3 include <svm.h>
<       3 include <sys/types.h>
257,266d111
<       3 include "Wm5Vector2.h"
<       3 include <xercesc/dom/DOMElement.hpp>
<       3 include <xercesc/dom/DOMNode.hpp>
<       3 include <xercesc/dom/DOMNodeList.hpp>
<       3 include <xercesc/parsers/XercesDOMParser.hpp>
<       4 include <assert.h>
<       4 include <boost/math/distributions.hpp>
<       4 include <boost/math/distributions/normal.hpp>
<       4 include <boost/random/uniform_int.hpp>
<       4 include <boost/random/variate_generator.hpp>
268d112
<       4 include <math.h>
270,274d113
<       4 include <stdio.h>
<       4 include <stdlib.h>
<       4 include <xercesc/util/PlatformUtils.hpp>
<       4 include <xercesc/util/XMLString.hpp>
<       5 include <boost/dynamic_bitset.hpp>
277d115
<       6 include <boost/random/mersenne_twister.hpp>
279,286d116
<       6 include <xercesc/sax2/Attributes.hpp>
<       7 include <boost/bind.hpp>
<       7 include <boost/regex.hpp>
<       7 include <Eigen/Core>
<       7 include <unsupported/Eigen/NonLinearOptimization>
<       9 include <boost/lexical_cast.hpp>
<       9 include <boost/unordered_map.hpp>
<       9 include <QtCore/QString>
292d121
<      12 include <omp.h>
297d125
<      17 include <boost/shared_ptr.hpp>
299d126
<      19 include <boost/math/special_functions/fpclassify.hpp>

so it looks like we did not do such a bad job at keeping out Qt from the core openms but there are 9 instances of QtString in there and quite a bit of general Qt. When looking at these includes individually and where they come from, they are mostly boost, xerces and Qt:

$ grep -R '#include'  src/openms/    | grep -v '<OpenMS' | cut -f 2 -d '#' | sort |  grep -v 'hpp>'  | grep -v '\.h>' | grep -v '\.h"' | grep -v '<Qt' | grep -v Eigen > /tmp/std_includes
$ grep -R '#include'  src/openms/   | grep -v '<OpenMS' | cut -f 2 -d '#' | sort   > /tmp/all_includes
$ diff /tmp/all_includes  /tmp/std_includes  | grep '^<' | grep -v '"' | cut -f 3 -d '<' | cut -f 1 -d "/" | cut -f 1 -d ">" | sort | uniq -c | sort -nr
    169 boost
     56 xercesc
     31 QtCore
     12 omp.h
     11 Eigen
     10 sys
      8 unsupported
      8 seqan
      5 QtNetwork
      4 stdlib.h
      4 stdio.h
      4 math.h
      4 assert.h
      3 unistd.h
      3 svm.h
      3 eigen3
      2 zlib.h
      2 windows.h
      2 utime.h
      2 QtGui
      2 mach
      1 Wm5Vector2.h
      1 Wm5Math.h
      1 Wm5IntpAkimaNonuniform1.h
      1 Wm5ApprLineFit2.h
      1 time.h
      1 stdint.h
      1 QCoreApplication.h
      1 process.h
      1 mach-o
      1 glpk.h
      1 errno.h
      1 CrawdadWrapper.h
      1 bzlib.h

So overall it looks like there is quite a substantial amount of Qt already in there.

Here it is for the gui part

$ grep -R '#include'  src/openms_gui/   | grep -v '<OpenMS' | cut -f 2 -d '#' | sort | uniq  -c | sort -n  > /tmp/all_includes
$ grep -R '#include'  src/openms_gui/    | grep -v '<OpenMS' | cut -f 2 -d '#' | sort | uniq -c | sort -n | grep -v 'hpp>'  | grep -v '\.h>' | grep -v '\.h"' | grep -v '<Qt' | grep -v Eigen > /tmp/std_includes
$ diff /tmp/all_includes  /tmp/std_includes 3d2
<       1 include <boost/shared_ptr.hpp>
23,28d21
<       1 include <Qt>
<       1 include <QtCore>
<       1 include <QtCore/QDate>
<       1 include <QtCore/QPoint>
<       1 include <QtCore/QRegExp>
<       1 include <QtCore/QTimer>
30,56d22
<       1 include <QtGui/QActionGroup>
<       1 include <QtGui/QBrush>
<       1 include <QtGui/QColor>
<       1 include <QtGui/QDialogButtonBox>
<       1 include <QtGui/QDoubleValidator>
<       1 include <QtGui/QDrag>
<       1 include <QtGui/QGraphicsSceneContextMenuEvent>
<       1 include <QtGui/QGraphicsSceneMouseEvent>
<       1 include <QtGui/QGraphicsView>
<       1 include <QtGui/QGroupBox>
<       1 include <QtGui/QImage>
<       1 include <QtGui/QImageWriter>
<       1 include <QtGui/QIntValidator>
<       1 include <QtGui/QItemSelection>
<       1 include <QtGui/QPainter>
<       1 include <QtGui/QPen>
<       1 include <QtGui/QPolygon>
<       1 include <QtGui/QRubberBand>
<       1 include <QtGui/QShortcut>
<       1 include <QtGui/QSpacerItem>
<       1 include <QtGui/QSpinBox>
<       1 include <QtGui/QSplitter>
<       1 include <QtGui/QStackedWidget>
<       1 include <QtGui/QTableWidget>
<       1 include <QtGui/QWheelEvent>
<       1 include <QtOpenGL/QGLWidget>
<       1 include <QtSvg/QtSvg>
66,70d31
<       2 include <QtCore/QMap>
<       2 include <QtCore/QRectF>
<       2 include <QtCore/QSettings>
<       2 include <QtCore/QTextStream>
<       2 include <QtCore/QVector>
72,94d32
<       2 include <QtGui/QBitmap>
<       2 include <QtGui/QColor>
<       2 include <QtGui/QColorDialog>
<       2 include <QtGui/QCompleter>
<       2 include <QtGui/QContextMenuEvent>
<       2 include <QtGui/QDesktopServices>
<       2 include <QtGui/QDirModel>
<       2 include <QtGui/QDragEnterEvent>
<       2 include <QtGui/QDragMoveEvent>
<       2 include <QtGui/QDropEvent>
<       2 include <QtGui/QFontMetrics>
<       2 include <QtGui/QGraphicsItem>
<       2 include <QtGui/QGraphicsScene>
<       2 include <QtGui/QItemDelegate>
<       2 include <QtGui/QKeyEvent>
<       2 include <QtGui/QLayout>
<       2 include <QtGui/QPixmap>
<       2 include <QtGui/QRadioButton>
<       2 include <QtGui/QResizeEvent>
<       2 include <QtGui/QScrollBar>
<       2 include <QtGui/QStyleFactory>
<       2 include <QtGui/QTabBar>
<       2 include <QtSvg/QSvgGenerator>
98,99d35
<       2 include <stdio.h>
<       2 include <stdlib.h>
104,106d39
<       3 include <QtCore/QObject>
<       3 include <QtCore/QSet>
<       3 include <QtCore/QTime>
108,117d40
<       3 include <QtGui/QDesktopWidget>
<       3 include <QtGui/QDockWidget>
<       3 include <QtGui/QListWidgetItem>
<       3 include <QtGui/QPainterPath>
<       3 include <QtGui/QStatusBar>
<       3 include <QtGui/QToolButton>
<       3 include <QtGui/QToolTip>
<       3 include <QtGui/QTreeWidgetItem>
<       3 include <QtGui/QWhatsThis>
<       3 include <QtGui/QWorkspace>
120d42
<       4 include <boost/math/special_functions/fpclassify.hpp>
122,128d43
<       4 include <QtCore/QMimeData>
<       4 include <QtCore/QUrl>
<       4 include <QtGui/QHeaderView>
<       4 include <QtGui/QMainWindow>
<       4 include <QtGui/QMenuBar>
<       4 include <QtGui/QToolBar>
<       4 include <QtGui/QVBoxLayout>
130,134d44
<       5 include <QtCore/QFile>
<       5 include <QtCore/QPoint>
<       5 include <QtCore/QProcess>
<       5 include <QtGui/QCloseEvent>
<       5 include <QtGui/QInputDialog>
136,158d45
<       6 include <QtCore/QFileInfo>
<       6 include <QtCore/QString>
<       6 include <QtCore/QStringList>
<       6 include <QtGui/QButtonGroup>
<       6 include <QtGui/QHBoxLayout>
<       7 include <QtGui/QApplication>
<       7 include <QtGui/QDialog>
<       7 include <QtGui/QListWidget>
<       7 include <QtGui/QPaintEvent>
<       7 include <QtGui/QSplashScreen>
<       7 include <QtGui/QWidget>
<       9 include <QtCore/QDir>
<       9 include <QtGui/QCheckBox>
<       9 include <QtGui/QMouseEvent>
<       9 include <QtGui/QTreeWidget>
<      10 include <QtGui/QLabel>
<      10 include <QtGui/QValidator>
<      12 include <QtGui/QGridLayout>
<      15 include <QtGui/QPushButton>
<      17 include <QtGui/QMenu>
<      17 include <QtGui/QPainter>
<      18 include <QtGui/QFileDialog>
<      18 include <QtGui/QTextEdit>
160,162d46
<      26 include <QtGui/QMessageBox>
<      28 include <QtGui/QComboBox>
<      40 include <QtGui/QLineEdit>

[timosachsenberg]

I will reopen a related issue