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

case sensitive value refs #3210

Merged

Conversation

agrrr3
Copy link
Contributor

@agrrr3 agrrr3 commented Oct 28, 2020

This PR intends to implement case sensitive parsing in FOCS.

forum discussion

State: halfway through (?) - opened the PR in order to trigger build and to create awareness

@o01eg o01eg added category:refactoring The Issue/PR describes or contains an improved implementation. component:content scripting The Issue/PR deals with the FOCS language, turn events or the universe generator. labels Oct 28, 2020
@o01eg o01eg added this to the v0.5 milestone Oct 28, 2020
@geoffthemedio geoffthemedio marked this pull request as draft October 28, 2020 15:04
@agrrr3
Copy link
Contributor Author

agrrr3 commented Oct 31, 2020

how do we proceed with this? cant really playtest much, probably some corner cases will be broken. squash and merge?

@agrrr3
Copy link
Contributor Author

agrrr3 commented Oct 31, 2020

Also rules are:

  • labels/parameter names lowercase (e.g. name = "BLA", buildcost)
  • definition names CamelCase (e.g. ShipDesign)
  • containers are CamelCase Source, Target, *Candidate
  • valuerefs are CamelCaseID - e.g. Source.X, Target.DesignID - also immediate value flags Value and Value()
  • arithmetic operations are lowercase (abs,min,max, ceil, round, cos, log, sign...) - may be these should be different. Max(enums) is not treated as arithmetics but like valuerefs.
  • conditionas are CamelCase, e.g. Ship, HasSpecialSinceTurn
  • effects are CamelCase, e.g. SetEmpireMeter
  • statistics are CamelCase - e.g. Statistics Max - could also be upper Statistics MAX
  • And, Or, Not - could also be AND, OR, NOT - especially NOT could make sense as it is easy to miss
  • Enums are CamelCase: Hostile, Tiny, GasGiant, Basic,...

probably i should change combatTargets to combattargets

@geoffthemedio
Copy link
Member

how do we proceed with this? cant really playtest much, probably some corner cases will be broken.

I don't think much "play" testing is needed. Main thing is whether the parse is successful, and are all the checksums the same. The classes created and enum values set shouldn't have changed, and I don't think you changed anything in ValueRefs.cpp so all the matched strings should be consistent for anything that was working...

squash and merge?

No need to squash. Just needs to be verified that everything parses correctly now.

@agrrr3
Copy link
Contributor Author

agrrr3 commented Oct 31, 2020

I don't think much "play" testing is needed. Main thing is whether the parse is successful, and are all the checksums the same. The classes created and enum values set shouldn't have changed, and I don't think you changed anything in ValueRefs.cpp so all the matched strings should be consistent for anything that was working...

everything parses. but how do i check the checksums?

@geoffthemedio
Copy link
Member

I guess comment out https://github.com/freeorion/freeorion/blob/master/client/human/HumanClientApp.cpp#L478 rebuild, quickstart, and check the server log for lines with checksum in them, like

22:08:01.698528 {0x000024e0} [debug] server : namedvaluerefmanager.cpp:111 : NamedValueRefManager checksum: 2045214
22:08:01.734536 {0x000024e0} [debug] server : shiphull.cpp:296 : ShipHullManager checksum: 6216434
22:08:01.756541 {0x000024e0} [debug] server : shippart.cpp:444 : ShipPartManager checksum: 439320
22:08:01.756541 {0x000024e0} [debug] server : shipdesign.cpp:865 : PredefinedShipDesignManager checksum: 862040
22:08:01.908575 {0x000024e0} [debug] server : species.cpp:588 : SpeciesManager checksum: 7766527
22:08:01.920578 {0x000024e0} [debug] server : tech.cpp:682 : TechManager checksum: 4875169

@o01eg
Copy link
Contributor

o01eg commented Oct 31, 2020

Unittests are also failing in the case of mismatched checksums.

@geoffthemedio
Copy link
Member

There wouldn't be a mismatch if the parser used by both client and server are modified the same way...

@agrrr3
Copy link
Contributor Author

agrrr3 commented Nov 2, 2020

ok, checksums are the same if applying the PR or not

so ready to merge i guess

@agrrr3
Copy link
Contributor Author

agrrr3 commented Nov 2, 2020

got merge conflicts. will rebase and repush

@agrrr3 agrrr3 force-pushed the 20201027_CaseInsensitiveValueRefs branch from 2a1574f to 6e63b91 Compare November 2, 2020 21:26
@agrrr3 agrrr3 marked this pull request as ready for review November 2, 2020 23:15
@agrrr3
Copy link
Contributor Author

agrrr3 commented Nov 2, 2020

good to go IMHO

@geoffthemedio geoffthemedio merged commit 7323397 into freeorion:master Nov 4, 2020
@geoffthemedio geoffthemedio added the status:merged All relevant commits of this PR were merged into the master development branch. label Nov 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:refactoring The Issue/PR describes or contains an improved implementation. component:content scripting The Issue/PR deals with the FOCS language, turn events or the universe generator. status:merged All relevant commits of this PR were merged into the master development branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants