Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Namespace and reorganize liblcf #361

Merged
merged 23 commits into from May 25, 2020
Merged

Conversation

fmatthew5876
Copy link
Contributor

@fmatthew5876 fmatthew5876 commented Apr 3, 2020

Depends on: EasyRPG/Player#2099

Fix: #342
Fix: #362

This PR does the following:

  • Put all liblcf symbols in ::lcf namespace
  • Put all rpg symbols in ::lcf::rpg namespace
  • Put all public header files in src/lcf/ or src/generated/lcf subdirectory. They now must be included like #include "lcf/data.h"
  • Rename public rpg headers from rpg_*.h to lcf/rpg/*.h
  • Keep all private headers and source files in src/
  • Install public headers to $(includedir)/lcf and change pkgconfig to use $(includedir) directly in cflags
  • Don't install private headers

Testing:

  • Test install works in cmake
  • Test install works in automake
  • Test that cmake and automake produce same installations
  • Build player against installation using automake / pkgconfig
  • Build player against installation using cmake files
  • Build player with local liblcf on linux
  • Build player with local liblcf on windows
  • Create a dist tarball and check that it compiles with autotools
  • Create a dist tarball and check that it compiles with cmake

Here is an example install from cmake

.
./lib
./lib/liblcf.so
./lib/liblcf.a
./lib/liblcf.so.0.0.0
./lib/cmake
./lib/cmake/liblcf
./lib/cmake/liblcf/FindICU.cmake
./lib/cmake/liblcf/liblcf-config.cmake
./lib/cmake/liblcf/FindEXPAT.cmake
./lib/liblcf.la
./lib/pkgconfig
./lib/pkgconfig/liblcf.pc
./lib/liblcf.so.0
./include
./include/lcf
./include/lcf/rpg_attribute.h
./include/lcf/rpg_save.h
./include/lcf/rpg_savepartylocation.h
./include/lcf/lsd_chunks.h
./include/lcf/rpg_savepicture.h
./include/lcf/rpg_start.h
./include/lcf/rpg_music.h
./include/lcf/rpg_saveeventexecstate.h
./include/lcf/rpg_treemap.h
./include/lcf/rpg_system.h
./include/lcf/rpg_savemapevent.h
./include/lcf/ldb_chunks.h
./include/lcf/rpg_battleranimationdata.h
./include/lcf/rpg_class.h
./include/lcf/rpg_switch.h
./include/lcf/rpg_event.h
./include/lcf/flag_set.h
./include/lcf/rpg_saveeventexecframe.h
./include/lcf/scope_guard.h
./include/lcf/rpg_movecommand.h
./include/lcf/rpg_equipment.h
./include/lcf/rpg_eventpagecondition.h
./include/lcf/data.h
./include/lcf/rpg_animation.h
./include/lcf/rpg_savetarget.h
./include/lcf/lmu_chunks.h
./include/lcf/rpg_variable.h
./include/lcf/rpg_parameters.h
./include/lcf/lcf_saveopt.h
./include/lcf/rpg_troopmember.h
./include/lcf/lsd_reader.h
./include/lcf/rpg_actor.h
./include/lcf/rpg_animationframe.h
./include/lcf/rpg_savemapeventbase.h
./include/lcf/rpg_map.h
./include/lcf/enum_tags.h
./include/lcf/rpg_savevehiclelocation.h
./include/lcf/rpg_skill.h
./include/lcf/rpg_itemanimation.h
./include/lcf/rpg_savepanorama.h
./include/lcf/rpg_savemapinfo.h
./include/lcf/rpg_eventpage.h
./include/lcf/encoder.h
./include/lcf/rpg_battlecommand.h
./include/lcf/rpg_trooppage.h
./include/lcf/rpg_commonevent.h
./include/lcf/rpg_enemy.h
./include/lcf/rpg_mapinfo.h
./include/lcf/rpg_trooppagecondition.h
./include/lcf/rpg_learning.h
./include/lcf/ldb_reader.h
./include/lcf/reader_xml.h
./include/lcf/rpg_battleranimation.h
./include/lcf/rpg_rect.h
./include/lcf/rpg_terrain.h
./include/lcf/rpg_savetitle.h
./include/lcf/lmt_reader.h
./include/lcf/reader_util.h
./include/lcf/ini.h
./include/lcf/rpg_battlecommands.h
./include/lcf/rpg_animationcelldata.h
./include/lcf/rpg_saveinventory.h
./include/lcf/rpg_animationtiming.h
./include/lcf/rpg_savecommonevent.h
./include/lcf/reader_lcf.h
./include/lcf/rpg_savesystem.h
./include/lcf/rpg_troop.h
./include/lcf/inireader.h
./include/lcf/rpg_moveroute.h
./include/lcf/rpg_item.h
./include/lcf/rpg_testbattler.h
./include/lcf/rpg_terms.h
./include/lcf/lmu_reader.h
./include/lcf/rpg_database.h
./include/lcf/lmt_chunks.h
./include/lcf/rpg_encounter.h
./include/lcf/rpg_saveeasyrpgdata.h
./include/lcf/writer_lcf.h
./include/lcf/rpg_enemyaction.h
./include/lcf/rpg_saveactor.h
./include/lcf/rpg_eventcommand.h
./include/lcf/rpg_battleranimationextension.h
./include/lcf/writer_xml.h
./include/lcf/rpg_state.h
./include/lcf/rpg_chipset.h
./include/lcf/rpg_sound.h
./include/lcf/command_codes.h
./include/lcf/config.h
./include/lcf/rpg_savescreen.h
./share
./share/mime
./share/mime/packages
./share/mime/packages/liblcf.xml
./share/mime/XMLnamespaces
./share/mime/generic-icons
./share/mime/treemagic
./share/mime/aliases
./share/mime/globs2
./share/mime/version
./share/mime/subclasses
./share/mime/application
./share/mime/application/x-ldb.xml
./share/mime/application/x-lmt.xml
./share/mime/application/x-lsd.xml
./share/mime/application/x-lmu.xml
./share/mime/mime.cache
./share/mime/globs
./share/mime/magic
./share/mime/types
./share/mime/icons

@fmatthew5876 fmatthew5876 changed the title Namespace liblcf Namespace and reorganize liblcf Apr 3, 2020
@fmatthew5876
Copy link
Contributor Author

I'm still testing this and working out edge cases. But the general approach is ready for review.

Let me know if you agree with this path or would suggest changes.

@fmatthew5876
Copy link
Contributor Author

I've fully tested this building on linux (see list in original post). This is ready now unless we want to change things.

@fmatthew5876
Copy link
Contributor Author

fmatthew5876 commented Apr 3, 2020

I added a fix for #362

Since LCF_SUPPORT_XML is needed in reader_xml.h. I changed cmake to also generated a config.h which defines this.

Now whether liblcf was built with autotools or cmake, external applications linking against it can know whether or not it has ICU and/or XML support.

@fmatthew5876
Copy link
Contributor Author

This has now been fully tested on windows and linux using the associated player PR.

@Ghabry
Copy link
Member

Ghabry commented Apr 22, 2020

0.6.2 is near and this should land quite soon after.


Question is if there should be directly more logical folder grouping as e.g. the MSVC Generator through CMake does:

Unbenannt

Maybe also get rid of file prefix (also planned for Player when the Makefiles are gone):

generated/lmt/lmt_encounter.cpp -> generated/lmt/encounter.cpp


Namespacing as you asked about RPG namespace:

Currently we have 5 prefixes: ldb, lmu, lsd, lmt, rpg. But it seems that ldb, lsd, lmt and lmu are just for the template-foo in cpp files, so there are no changes for outside users there.

About RPG: Imo lcf::RPG looks strange because of the full lower-upper case. Should be lcf::rpg. Not sure about dropping rpg namespace completely because the namespace adds semantic information: "Everything in rpg is part of the data structure".


Do you already have plans about making enums -> enum class?

@fmatthew5876
Copy link
Contributor Author

fmatthew5876 commented Apr 22, 2020

0.6.2 is near and this should land quite soon after.

Question is if there should be directly more logical folder grouping as e.g. the MSVC Generator through CMake does:

Maybe also get rid of file prefix (also planned for Player when the Makefiles are gone):

generated/lmt/lmt_encounter.cpp -> generated/lmt/encounter.cpp

Sure we can move this stuff around. I'm not convinced we'll want to have lcf::ldb, lcf::lsd, etc.. namespaces though. Seems like overkill maybe?

About RPG: Imo lcf::RPG looks strange because of the full lower-upper case. Should be lcf::rpg. Not sure about dropping rpg namespace completely because the namespace adds semantic information: "Everything in rpg is part of the data structure".

I agree about lcf::rpg:: namespace. All the game "data" is nice to have in it's own space.

Do you already have plans about making enums -> enum class?

I'm on the fence about this one. We do a lot of enum <-> int conversions and enum class adds a lot of noise with needing to do static_cast everywhere and also fully scoping the enum name. If we go with enum classes, we may want to take the enums out from being members of their respective classes so we don't have to say lcf::rpg::ClassName::EnumName::EnumValue. Need to think about it more.

I will take another pass at this PR this weekend.

@Ghabry
Copy link
Member

Ghabry commented Apr 22, 2020

I'm not convinced we'll want to have lcf::ldb, lcf::lsd, etc.. namespaces though

Because this is not public API: No, namespacing is not needed.
Also too deep nesting looks ugly before C++17 because you can't do "namespace A::B::C {}"

I'm on the fence about this one. We do a lot of enum <-> int conversions

Right the casting is very annoying with enum classes. Lets focus on the other parts first.

@fmatthew5876
Copy link
Contributor Author

Rebased, but I need to review and retest everything. Also add lcf::rpg

@fmatthew5876 fmatthew5876 marked this pull request as draft April 26, 2020 23:05
@fmatthew5876 fmatthew5876 marked this pull request as ready for review April 28, 2020 01:31
@fmatthew5876
Copy link
Contributor Author

fmatthew5876 commented Apr 28, 2020

I've made the following changes

  • Renamed RPG:: to lcf::rpg::

  • Move headers rpg_*.h to lcf/rpg/*.h

  • Made MoveCommand::Code and EventCommand::Code enum classes

This gets rid of the ugly struct Code { enum Index { ... }}; thing we have going now. The price of this is more static casts in player (you can see them in the player PR).

Since these really are codes and not meant to be used as numbers, and they already use this nested style, this one seemed ok to migrate to enum class.

The others we can consider in a follow up PR.

@fmatthew5876 fmatthew5876 marked this pull request as draft April 28, 2020 02:29
@fmatthew5876 fmatthew5876 marked this pull request as ready for review April 28, 2020 02:29
@fmatthew5876
Copy link
Contributor Author

fmatthew5876 commented Apr 28, 2020

I put the ldb, lmt, lmu, and lsd public headers in their own sub directories. The final installation looks much nicer that way.

Not happy with the cmake/automake hacks to make this work. Is there a better way?

From the public API standpoint, this PR is done unless review comments want changes.

Not sure about putting all the ldb, lmt, lmu, and lsd source files in subdirectories. Do we really want this? I don't have a strong opinion as it's not part of the public interface.

@Ghabry
Copy link
Member

Ghabry commented Apr 28, 2020

I like it this way 👍

Most important part is to avoid another move-around in the public interface (requiring player updates). Internal interface can be moved around in a later PR.

The extra work for install really looks ugly. No idea yet if this is solvable in a better way, will think about it.

* Install all public headers to $(includedir)/lcf
* Don't install private headers like reader_struct.h
* Modify pkgconfig to reflect new include dir
* Modify cmake file to reflect new include dir
* Add a config.h for cmake which defines XML and ICU
* Use config.h from cmake instead of setting compile definitions
* Autotools: don't put config.h directly in src tree
* Autotools/cmake: install config.h
* Remove lcf_options.h and include lcf/config.h directly, and do it
  first

Fix: EasyRPG#362
* Use cmakedefine01 style in both cmake and autoconf
* Remove autoconf AC_HEADER hacks
Makefile.am Outdated Show resolved Hide resolved
Copy link
Member

@Ghabry Ghabry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build fails for "-DDISABLE_XML=ON"

@Ghabry
Copy link
Member

Ghabry commented May 14, 2020

liblcf-config.cmake is broken. Needs a different file in NAMES. e.g. "lcf/reader_lcf.h" works

Copy link
Member

@Ghabry Ghabry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the hughest change to liblcf in a while. But this is the first and last time we move so many files around ;)

@carstene1ns carstene1ns merged commit d30bad9 into EasyRPG:master May 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

lcf_options.h HAVE_CONFIG_H is not robust Namespace and organize lcf headers
4 participants