Differential D11828 Diff 31623 src/engine/orpostingiterator.cpp

Changeset View

Standalone View

src/engine/orpostingiterator.cpp

Show All 19 Lines
20		20
21	#include "orpostingiterator.h"	21		#include "orpostingiterator.h"
22		22
23	using namespace Baloo;	23		using namespace Baloo;
24		24
25	OrPostingIterator::OrPostingIterator(const QVector<PostingIterator*>& iterators)	25		OrPostingIterator::OrPostingIterator(const QVector<PostingIterator*>& iterators)
26	: m_iterators(iterators)	26		: m_iterators(iterators)
27	, m_docId(0)	27		, m_docId(0)
		28		, m_nextId(0)
			michaelhUnsubmitted Done `, m_nextId(ULONG_LONG_MAX)` ? michaelh: `, m_nextId(ULONG_LONG_MAX)` ?
			brunsAuthorUnsubmitted Done Currently the only reserved value is `0`, and I prefer to keep it as is. bruns: Currently the only reserved value is `0`, and I prefer to keep it as is.
28	{	29		{
		30		for (auto it = m_iterators.begin(), end = m_iterators.end(); it != end;) {
		31		/*
		32		* Check for null iterators
		33		* Preferably, these are not pushed to the list at all, but better be safe
		34		*/
		35		if (!(*it)) {
		36		it = m_iterators.erase(it);
		37		continue;
		38		}
		39
		40		auto docId = (*it)->next();
		41		// find smallest docId
		42		if (docId && (docId < m_nextId \|\| m_nextId == 0)) {
		43		m_nextId = docId;
		44		}
			michaelhUnsubmitted Done `m_nextId = (docId > 0) ? qMin(m_nextId, docId) : m_nextId;` ? michaelh: `m_nextId = (docId > 0) ? qMin(m_nextId, docId) : m_nextId;` ?
		45
		46		it++;
		47		}
29	}	48		}
30		49
31	OrPostingIterator::~OrPostingIterator()	50		OrPostingIterator::~OrPostingIterator()
32	{	51		{
33	qDeleteAll(m_iterators);	52		qDeleteAll(m_iterators);
34	}	53		}
35		54
36	quint64 OrPostingIterator::docId() const	55		quint64 OrPostingIterator::docId() const
37	{	56		{
38	return m_docId;	57		return m_docId;
39	}	58		}
40		59
41	quint64 OrPostingIterator::next()	60		quint64 OrPostingIterator::next()
42	{	61		{
43	m_docId = 0;	62		m_docId = m_nextId;
44	if (m_iterators.isEmpty()) {	63		m_nextId = 0;
		64
		65		if (!m_docId) {
45	return 0;	66		return 0;
			brunsAuthorUnsubmitted Done Move this to the constructor? bruns: Move this to the constructor?
46	}	67		}
47		68
			michaelhUnsubmitted Done Remove the above and instead ? if (m_nextId == ULONG_LONG_MAX) { m_docId = 0; return 0; } m_docId = m_nextId; ? michaelh: Remove the above and instead ? ``` if (m_nextId == ULONG_LONG_MAX) { m_docId = 0; return…
48	for (auto it = m_iterators.begin(), end = m_iterators.end(); it != end; it++) {	69		// advance all iterators which point to the lowest docId
			brunsAuthorUnsubmitted Done iterator may contain nullptr's bruns: iterator may contain nullptr's
		70		for (auto it = m_iterators.begin(); it != m_iterators.end(); ) {
49	PostingIterator* iter = *it;	71		PostingIterator* iter = *it;
			michaelhUnsubmitted Done Newbie question: Does std::unique_ptr make sense here? michaelh: Newbie question: Does std::unique_ptr make sense here?
			brunsAuthorUnsubmitted Done When writing from scratch today, yes. But this would not disallow nullptr (`std::make_unique<PostingIterator>(nullptr)` is fully valid) bruns: When writing from scratch today, yes. But this would not disallow nullptr (`std…
50	if (!iter) {
51	continue;
52	}
53		72
54	// First or last element	73		auto docId = iter->docId();
55	if (iter->docId() == 0 && iter->next() == 0) {	74		if (docId <= m_docId) {
		75		docId = iter->next();
		76		// remove element if iterator has reached the end
		77		if (docId == 0) {
56	delete iter;	78		delete iter;
57	*it = nullptr;	79		*it = nullptr;
			poboikoUnsubmitted Done I'm not really sure, do we actually need to set it to `nullptr`? It becomes inaccessible after next line poboiko: I'm not really sure, do we actually need to set it to `nullptr`? It becomes inaccessible after…
			brunsAuthorUnsubmitted Done It makes debugging a little bit nicer, erase is implemented as swap, and when setting it the vector only contains nullptrs after size() elements. bruns: It makes debugging a little bit nicer, erase is implemented as swap, and when setting it the…
		80		it = m_iterators.erase(it);
58	continue;	81		continue;
59	}	82		}
			poboikoUnsubmitted Done Usage of `auto` keyword (at least for docId) somehow made it a bit harder to read for me. Looks like Qt Coding Conventions agrees (it's fine for iterators, whose definition has a long-long-name and it's clear from the line that it's an iterator. But for ordinary `docId` I've began to think it's not `quint64`, but some weird class with large definition or something). poboiko: Usage of `auto` keyword (at least for docId) somehow made it a bit harder to read for me. Looks…
60
61	if (iter->docId() < m_docId \|\| m_docId == 0) {
62	m_docId = iter->docId();
63	}
64	}	83		}
65		84
66	for (auto it = m_iterators.cbegin(), end = m_iterators.cend(); it != end; it++) {	85		// check if the docId is the new lowest docId
67	PostingIterator* iter = *it;	86		if (((docId < m_nextId)) \|\| (m_nextId == 0)) {
			fvogtUnsubmitted Done Looks like most of those parens are unnecessary. fvogt: Looks like most of those parens are unnecessary.
			brunsAuthorUnsubmitted Done only one pair is, the others support readability, see Qt Coding Style: Use parentheses to group expressions bruns: only one pair is, the others support readability, see Qt Coding Style: > Use parentheses to…
68	if (iter && iter->docId() <= m_docId) {	87		m_nextId = docId;
69	iter->next();
70	}	88		}
			michaelhUnsubmitted Done `m_nextId = qMin(docId, m_nextId);` ? michaelh: `m_nextId = qMin(docId, m_nextId);` ?
		89
		90		it++;
71	}	91		}
72		92
73	return m_docId;	93		return m_docId;
74	}	94		}