Review Request: Add accessors for Solid Predicate internals previously only exposed in ctor's

Kevin Ottens ervin at kde.org
Wed Sep 2 18:48:45 BST 2009


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/1491/#review2237
-----------------------------------------------------------


I'm slightly torn on the general approach of "let's expose everything using an accessor". As if we make the internals grow the class will quickly become unmanageable with lots of accessors. That said, a single accessor returning a struct with only the relevant data is probably not going to cut it either... *sigh*

Otherwise the patch looks OK, I had a few comments though, please address them before a second round of review.


/trunk/KDE/kdelibs/solid/solid/predicate.h
<http://reviewboard.kde.org/r/1491/#comment1528>

    Hm, now that it is public, Type would probably be more appropriate. It's not about a single operator, but about the whole node of the predicate tree.
    
    Would make "AtomType" less out of place since this one is definitely not an operator.



/trunk/KDE/kdelibs/solid/solid/predicate.h
<http://reviewboard.kde.org/r/1491/#comment1529>

    Same comment as above.



/trunk/KDE/kdelibs/solid/solid/predicate.h
<http://reviewboard.kde.org/r/1491/#comment1530>

    There's a typo: s/aganist/against/



/trunk/KDE/kdelibs/solid/solid/predicate.h
<http://reviewboard.kde.org/r/1491/#comment1531>

    Should be firstOperand() and secondOperand().



/trunk/KDE/kdelibs/solid/solid/predicate.cpp
<http://reviewboard.kde.org/r/1491/#comment1532>

    I guess we'll die a painful death here if the pointer was 0. Check for it and return an invalid predicate in this case.
    
    Otherwise just return *d->operand1. The extra Predicate ctor isn't necessary IIRC.



/trunk/KDE/kdelibs/solid/solid/predicate.cpp
<http://reviewboard.kde.org/r/1491/#comment1533>

    Same as above.


- Kevin


On 2009-08-31 10:39:02, Ben Cooksley wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/1491/
> -----------------------------------------------------------
> 
> (Updated 2009-08-31 10:39:02)
> 
> 
> Review request for kdelibs and Kevin Ottens.
> 
> 
> Summary
> -------
> 
> Adds accessors for internal objects previously exposed in ctor's, and the OperatorType enum.
> This allows applications such as the Device Actions KCM to share the same parsing implementation as libsolid and prevents code duplication, etc.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdelibs/solid/solid/predicate.h 1017656 
>   /trunk/KDE/kdelibs/solid/solid/predicate.cpp 1017656 
> 
> Diff: http://reviewboard.kde.org/r/1491/diff
> 
> 
> Testing
> -------
> 
> Compiles.
> 
> 
> Thanks,
> 
> Ben
> 
>





More information about the kde-core-devel mailing list