<div dir="ltr"><div><div><div><div><div><div><div><div><div>Hi all,<br><br></div>MY opinion is that's not a really good way to count how many classes you will have and trying to have minimal classes.<br><br></div>Drawing a model with UML is not easy : it's not easy to have a good model from the beginning, it's not easy to name all members and methods correctly, and the worst is inheritance.<br>
</div>You can think you spend too much time to make the model. But you can't say if you will spend less or much time to change code because your model was too simple.<br><br></div>Because I don't know all the model of Gdal and the model can be "reset" I have some questions :<br>
<br></div><div>1) Why GDALMajorObject not inherit of GDALIMajorObject ? <br></div><div><br></div>2) Why all classes inherit (directly or not) of GDALMajorObject ? What do you store in it ? Is it possible to put it as a member in classes ? Have you consider templates ?<br>
<br></div>3) An interface for Drivers ? geName(), getLongName(), getDescription() ? Or maybe abstract ...<br><br></div>4) What is GDALDataset in your diagram ?<br><br></div>5) Where is the link between Driver (Gdal/Ogr) and Dataset (Raster/Vector) ? Member, templates ?<br>
<br></div>Florent<br></div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-03-25 21:14 GMT+01:00 Even Rouault <span dir="ltr"><<a href="mailto:even.rouault@mines-paris.org" target="_blank">even.rouault@mines-paris.org</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Selon Etienne Tourigny <<a href="mailto:etourigny.dev@gmail.com">etourigny.dev@gmail.com</a>>:<br>
<div class=""><br>
> I also (respectfully) think that there are too many classes (mostly the<br>
> abstract classes and interfaces) which make it a bit hard to understand.<br>
><br>
> I like Vincent's second suggestion to have 3 base Dataset classes<br>
> (GDALRasterDataset, GDALVectorDataset, GDALHybridDataset) which inherit<br>
> from GDALDataset, and an enum (vector,raster,hybrid) to easily query which<br>
> type the Dataset is (or which type its driver supports). You then subclass<br>
> the relevant Dataset class and it *should* work auto-magically for rasters.<br>
>  For example, a given Dataset which subclasses GDALDataset would be<br>
> migrated as  a subclass of GDALRasterDataset, which would imply that it<br>
> does not have a vector dataset. If the implementation changes to allow<br>
> vector, then you would change its parent and implement relevant vector<br>
> methods.<br>
><br>
> My view on this is KISS with a minimum number of classes...<br>
<br>
</div>Yes, the practice shows that the UML brainstorming leads to nowhere (at least<br>
with my current limited knowledge of C++ multi-inheritance)... So actually<br>
instead of creating new classes, I think that eventually there will be less<br>
classes... I'm not even sure about the interest of having subclasses of<br>
GDALDataset to distinguish between raster only, vector only or raster+vector. As<br>
I wrote previously that could be done as a driver metadata instread.<br>
<br>
So, for now, I've just "imported" the usefull methods of OGRDataSource into<br>
GDALDataset, and made a few changes here or there so that it works. Well except<br>
ogr_refcount.py, because of a funny discovery was that the ref counting of<br>
GDALDataset and OGRDataSource is (was) not the same. Starts at 1 for GDALDataset<br>
and 0 for OGRDataSource...<br>
<br>
The temporary result is in the following branch:<br>
<a href="https://github.com/rouault/gdal-1/commits/unification" target="_blank">https://github.com/rouault/gdal-1/commits/unification</a><br>
<br>
OGRDataSource is kept with just that :<br>
<br>
class CPL_DLL OGRDataSource : public GDALDataset<br>
{<br>
public:<br>
                        OGRDataSource();<br>
<br>
    virtual const char  *GetName() = 0;<br>
<br>
    static void         DestroyDataSource( OGRDataSource * );<br>
<br>
    OGRSFDriver        *GetOGRDriver() const;<br>
    void                SetOGRDriver( OGRSFDriver *poDriver );<br>
    virtual     const char* GetDriverName();<br>
<br>
protected:<br>
    friend class OGRSFDriverRegistrar;<br>
<br>
    OGRSFDriver        *m_poOGRDriver;<br>
<br>
};<br>
<br>
And it could eventually completely disappear (or maybe just remain as a<br>
convenience class if we don't want to touch existing drivers) if I manage to<br>
merge OGRSFDriver into GDALDriver, and get rid of OGRSFDriverRegistrar.<br>
<span class="HOEnZb"><font color="#888888"><br>
Even<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
> cheers<br>
> Etienne<br>
><br>
><br>
><br>
> On Tue, Mar 25, 2014 at 9:13 AM, Vincent Mora<br>
> <<a href="mailto:vincent.mora@oslandia.com">vincent.mora@oslandia.com</a>>wrote:<br>
><br>
> > On 25/03/2014 11:44, Even Rouault wrote:<br>
> ><br>
> >> Selon Vincent Mora <<a href="mailto:vincent.mora@oslandia.com">vincent.mora@oslandia.com</a>>:<br>
> >><br>
> >>  On 24/03/2014 21:46, Even Rouault wrote:<br>
> >>><br>
> >>>> Hi,<br>
> >>>><br>
> >>>> "Release soon, release early", so for people who like UML diagrams<br>
> >>>> (there<br>
> >>>><br>
> >>> is<br>
> >>><br>
> >>>> also a prototype of C++ classes for those who don't like UML very much),<br>
> >>>><br>
> >>> here's<br>
> >>><br>
> >>>> a blog entry with the outcome of my thoughts for a possible<br>
> >>>> re-organisation<br>
> >>>><br>
> >>> of<br>
> >>><br>
> >>>> the GDAL/OGR class hierarchy<br>
> >>>><br>
> >>>><br>
> >>>>  <a href="http://erouault.blogspot.ca/2014/03/draft-gdalogr-class-" target="_blank">http://erouault.blogspot.ca/2014/03/draft-gdalogr-class-</a><br>
> >> hierarchy-for-gdal.html<br>
> >><br>
> >>> I don't understand the need for HandleRasterData() and<br>
> >>> HandleVectorData().<br>
> >>><br>
> >>> Isn't inheriting from GDALEmptyRasterDataset or GDALEmptyVectorDataset<br>
> >>> sufficient to convey the information that the class doesn't handle those<br>
> >>> kind of datasets.<br>
> >>><br>
> >>> A call to GetLayerCount()/GetRasterCount() from the class user is<br>
> >>> sufficient to say "no vector/raster data here", and a dynamic _cast to<br>
> >>> GDALEmptyVectorDataset/GDALEmptyRasterDataset is even clearer to me, so<br>
> >>> HandleRasterData()/HandleVectorData() would be a third way to tell the<br>
> >>> same thing.<br>
> >>><br>
> >> Actually, when thinking more about this, this kind of information should<br>
> >> be at<br>
> >> the driver level, and not at the dataset level. The idea is that there<br>
> >> are use<br>
> >> cases where you want to know if a driver can handle only vector, only<br>
> >> raster, or<br>
> >> both. For example if you still want to have separate Open dialog boxes for<br>
> >> raster or vector.<br>
> >><br>
> >>> Also I'm not sure about the names GDALAbstract* since those are partial<br>
> >>> implementations.<br>
> >>><br>
> >> I know I'm not very good at naming. Do you have an alternative proposal ?<br>
> >> The<br>
> >> issue is that with my draft we end up with a lot of classes and<br>
> >> interfaces, so<br>
> >> it is not obvious to find a good name to reflect their content.<br>
> >><br>
> > Partial sound ok to me, but I not so good at naming  ?<br>
> ><br>
> > Considering the number of classes, I gave it some thoughts this morning<br>
> > and, like Dmitry, thought of merging the Abtract (Partial) into the<br>
> > interface before realizing that, even if it works (you can overload de<br>
> > default function in Empty) it's a bit ugly and I ended up preferring your<br>
> > solution.<br>
> ><br>
> > An altenative would be to have a Dataset with both aspects and provide<br>
> > three partial specialisations: one for vector (it will behave like empty<br>
> > raster) one for raster, and one for both. Code duplications could be<br>
> > avoided by implementing protected member functions in the Dataset class and<br>
> > simply calling them in the implementations of partial specialization. This<br>
> > solution avoid the diamond shaped inheritance diagram of hell :)<br>
> ><br>
> >    Even<br>
> >><br>
> >><br>
> >><br>
> > _______________________________________________<br>
> > gdal-dev mailing list<br>
> > <a href="mailto:gdal-dev@lists.osgeo.org">gdal-dev@lists.osgeo.org</a><br>
> > <a href="http://lists.osgeo.org/mailman/listinfo/gdal-dev" target="_blank">http://lists.osgeo.org/mailman/listinfo/gdal-dev</a><br>
> ><br>
><br>
<br>
<br>
_______________________________________________<br>
gdal-dev mailing list<br>
<a href="mailto:gdal-dev@lists.osgeo.org">gdal-dev@lists.osgeo.org</a><br>
<a href="http://lists.osgeo.org/mailman/listinfo/gdal-dev" target="_blank">http://lists.osgeo.org/mailman/listinfo/gdal-dev</a><br>
</div></div></blockquote></div><br></div>