search

LanguageMachines / libfolia

FoLiA library for C++
https://proycon.github.io/folia
GNU General Public License v3.0
15 stars 7 forks source link

Rethink the Class structure of libfolia #39

Closed kosloot closed 3 years ago

kosloot commented 4 years ago

To simplify the code (readability AND maintenance) it might be an idea to merge the FoliaElement and AbstractElement classes.

proycon commented 4 years ago

That would be similar to the solutions I use for both Python and Rust yeah (for the latter actually, I don't even have separate classes for the various elements anymore but just one).

kosloot commented 4 years ago

This might be counterproductive. FoliaElement was intended to be a pure virtual Class with only functions and no data. So a true interface class. AbstractElement should provide default implementations and true data members. I think this is still a good plan, but current things are a bit messy. A user of the library should be unaware of the AbstractElement class, and always use FoliaElement.

Also AbstractElement has for instance an _xlink map and other data members that are only useful for a small amount of derived classes. In C++ that means a waste of space. So it might even better to use virtual inheritance to have such data members only present for certain Classes. That would save space. But a lot of refactoring is necessary then. is it worthwhile?

proycon commented 4 years ago

The waste of space is definitely something worth reducing, I explicitly designed the rust library with that in mind (and it worked), but there I had the benefit of hindsight and could use a significantly different structure than what we use in libfolia and foliapy. Doing the same in libfolia is way too big of a refactoring project at this stage I'm afraid...

kosloot commented 4 years ago

Well, I experimented with shifting the xlink map to a seperate class. This meant some recoding, but it worked, and required only a recompilation of ucto, foliautils and frog, no code change. So this might be fruitful BUT: it might be not that easy to autogenerate the new dependencies in the FoLia properties file. More investigation is needed

kosloot commented 4 years ago

Conclusion for now: Leave FoliaElement as the (virtual) base class and AbstractElement as the implementation thereof. Main point of confusion are the template member in FoliaElement. those cannot be virtual, and have to be defined/implemented in AbstractElement too. Maybe look into that later.

kosloot commented 3 years ago

closing this as being less important after a lot of other refactorings.