D13711: Support import of ROOT (CERN) TH1 histograms

Fabian Kristof noreply at phabricator.kde.org
Mon Jun 25 11:17:50 UTC 2018


fkristof added inline comments.

INLINE COMMENTS

> ROOTFilter.cpp:168
> +	QVector<void*> dataContainer;
> +	int columnOffset = dataSource->prepareImport(dataContainer, importMode, imax - i0 + 1, 3,
> +	                                             {"Bin Center", "Content", "Error"},

columnOffSet can be const if you don't intend to change it.

> ROOTFilter.cpp:181
> +		DEBUG("reading row " << i);
> +		centerContainer[i - i0] = i > 0 && i < nbins - 1 ? 0.5 * (bins[i].lowedge + bins[i + 1].lowedge)
> +		                                            : i == 0 ? bins.front().lowedge // -infinity

I suggest you to group these conditions with parenthesis so it's easier to understand them.

> ROOTFilter.cpp:191
> +
> +void ROOTFilterPrivate::write(const QString & fileName, AbstractDataSource* dataSource) {
> +	Q_UNUSED(fileName);

const QString & fileName

Extra space, remove it

> ROOTFilter.cpp:201
> +	QStringList histList;
> +	for (auto & hist : currentROOTHist->ListHistograms()) {
> +		histList << QString::fromStdString(hist);

This should be

  const auto&

> ROOTFilterPrivate.h:70
> +	 */
> +	std::vector<std::string> ListHistograms() const;
> +

The code should follow our conventions. We use camelCase in the naming.

> ROOTOptionsWidget.cpp:72
> +
> +	for (auto item: ui.twContent->selectedItems())
> +		names << item->text();

const auto&

REPOSITORY
  R262 LabPlot

REVISION DETAIL
  https://phabricator.kde.org/D13711

To: croick, sgerlach, asemke, fkristof
Cc: kde-edu, narvaez, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20180625/54b5f186/attachment.html>


More information about the kde-edu mailing list