Differential D11826

Simplify Term operator&& and ||
ClosedPublic
Actions

Authored by bruns on Mar 31 2018, 3:41 AM.

Details

Reviewers
michaelh
Group Reviewers
Baloo
Commits
R293:b0dcc732ca98: Simplify Term operator&& and ||
Summary

The Term(Term, Operator, Term) constructor is identical to the three
separate calls, but saves two separate function calls.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>

Try to merge terms with identical operations

(a AND b) AND (c OR d) can be merged as (a AND b AND (c OR D), i.e. 2
instead of 3 intermediate AND/OR terms.
This is especially useful if terms are combined with a && b && c && d,
which can be combined into one AND term with 4 subterms, instead of
3 AND terms with 2 subterms each (((a AND b) AND c) AND d).

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>

Test Plan

ctest -V -R advancedqueryparser

38: QDEBUG : AdvancedQueryParserTest::testOptimizedLogic(a && b && c && d)   result term ( AND ( : a (QString)) ( : b (QString)) ( : c (QString)) ( : d (QString)) )
38: QDEBUG : AdvancedQueryParserTest::testOptimizedLogic(a && b && c && d) expected term ( AND ( : a (QString)) ( : b (QString)) ( : c (QString)) ( : d (QString)) )
38: PASS   : AdvancedQueryParserTest::testOptimizedLogic(a && b && c && d)
38: QDEBUG : AdvancedQueryParserTest::testOptimizedLogic((a && b) && (c || d))   result term ( AND ( : a (QString)) ( : b (QString)) ( OR ( : c (QString)) ( : d (QString)) ) )
38: QDEBUG : AdvancedQueryParserTest::testOptimizedLogic((a && b) && (c || d)) expected term ( AND ( : a (QString)) ( : b (QString)) ( OR ( : c (QString)) ( : d (QString)) ) )
38: PASS   : AdvancedQueryParserTest::testOptimizedLogic((a && b) && (c || d))

Diff Detail

Repository
R293 Baloo
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
bruns created this revision.Mar 31 2018, 3:41 AM
Restricted Application added projects: Frameworks, Baloo. · View Herald TranscriptMar 31 2018, 3:41 AM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
bruns requested review of this revision.Mar 31 2018, 3:41 AM
bruns added a reviewer: Baloo.Mar 31 2018, 4:26 AM
michaelh added a subscriber: michaelh.Mar 31 2018, 9:09 AM

This part of baloo is still unknown territory to me. Please be patient until I understood what is going on here.
If you want to, ignore my inline comments. Then this file will violate the coding style rules at least consistently.

src/lib/term.cpp
121

<Coding style>Please use braces

129

</Coding style>

bruns updated this revision to Diff 31160.Apr 2 2018, 5:01 PM

Fix coding style ({/} on single lines)

bruns marked 2 inline comments as done.Apr 2 2018, 5:02 PM
bruns added a comment.Apr 2 2018, 6:09 PM

Fixed coding style

michaelh added a comment.Apr 5 2018, 12:10 PM

Regard this as accepted, but please see D11907 for landing sequence.

michaelh accepted this revision.Apr 5 2018, 9:37 PM
This revision is now accepted and ready to land.Apr 5 2018, 9:37 PM
michaelh added a comment.Apr 5 2018, 9:41 PM

Oops, accepted it too quickly, forgot about the failmessages.

bruns updated this revision to Diff 31541.Apr 7 2018, 1:24 AM
bruns edited the test plan for this revision. (Show Details)
michaelh added a comment.Apr 7 2018, 5:48 AM

Couldn't test. This has to rebased first. It doesn't really matter as I had tested this before. Just land it.
Wrt "Test Plan": Most people just write 'make test' otherwise the commit message will be very long.

bruns updated this revision to Diff 31581.Apr 7 2018, 11:18 AM

rebased

Closed by commit R293:b0dcc732ca98: Simplify Term operator&& and || (authored by bruns). · Explain WhyApr 7 2018, 11:20 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 31582

View Options

autotests/unit/lib/advancedqueryparsertest.cpp

Loading...
View Options

src/lib/term.h

Loading...
View Options

src/lib/term.cpp

Loading...