patchFreeciv - Patches: patch #3879, Remove "negated" reqs{}...

 
 
Show feedback again

patch #3879: Remove "negated" reqs{} field in favor of something semantically nicer

Submitted by:  Emmet Hikory <persia>
Submitted on:  Wed 24 Apr 2013 12:52:01 AM UTC  
 
Category: generalPriority: 5 - Normal
Status: DonePrivacy: Public
Assigned to: Marko Lindqvist <cazfi>Open/Closed: Closed
Planned Release: 2.6.0

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Mon 13 May 2013 09:32:38 PM UTC, SVN revision 22861:

Replaced requirement property "negated" with opposite "present"
This means that the value needs not to be double-negative.

Patch by Emmet Hikory

As this is first ruleset format change since S2_5 was branched,
also bumped capability string to make it different between branches.

See patch #3879

(Browse SVN revision 22861)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Thu 09 May 2013 09:47:54 PM UTC, comment #8:

Further experimentation showed that this patch broke effects network packets. Updated patch attached with sense flipped for generated packets. As a side note, it appears that effects requirements could not previously handle "negated" correctly in the client, which bug is preserved with the migration to "present", to be handled in another future ticket.

(file #17944)

Emmet Hikory <persia>
Project Member
Thu 09 May 2013 08:32:35 AM UTC, comment #7:

> With most terms the problem seems to be that they don't make it
> clear that FALSE -> requirement is not allowed to be present,
> rather than just being not really a requirement.


Idea for potential future ticket: What if it wasn't boolean (TRUE/FALSE) in ruleset format, but the strings used there were "present"/"absent", which ruleset loading code then would just convert to internal boolean representation?

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Thu 09 May 2013 08:23:11 AM UTC, comment #6:

Updated patch attached against SVN 22836.

(file #17938)

Emmet Hikory <persia>
Project Member
Thu 09 May 2013 08:07:40 AM UTC, comment #5:

> This discussion appears to be complete.


Yes. This is also rather important change to get in...

> The attached patch is stacked above patch #3835 as it includes
> changes to code changed by that patch, but this is mostly for
> convenience


...so I would prefer to drop the patch #3835 dependency, and to go forward with this (#3879) while #3835 is still a bit undecided.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Thu 09 May 2013 06:31:40 AM UTC, comment #4:

This discussion appears to be complete. Using a positive entry, as this appears to be preferred by 3/3 of those expressing a preference. Selected the term "present", as this was in the preference lists of 2/2 of those expressing a preference. Rebasing above current trunk now includes several more ruleset changes to reflect SVN revision 22794.

The attached patch is stacked above patch #3835 as it includes changes to code changed by that patch, but this is mostly for convenience (not needing to post an update there). The relevant changes could be moved without significant impact to functionality.

(file #17936)

Emmet Hikory <persia>
Project Member
Wed 24 Apr 2013 04:09:53 PM UTC, comment #3:

Seeing "true" makes me think of "boolean": it makes the C code a little odd ("bool boolean", "if (preq->boolean)", "boolean ? "boolean" : ""), but may be easy to understand for ruleset authors.

Emmet Hikory <persia>
Project Member
Wed 24 Apr 2013 08:43:26 AM UTC, comment #2:

> "present", "active", "exists", "condition", "test", "asserted",
> "found"


With most terms the problem seems to be that they don't make it clear that FALSE -> requirement is not allowed to be present, rather than just being not really a requirement. Though people should realize that there's no point in requirement that is not required...

If the above terms I prefer "present" and "active" (ones that I thought up myself before seeing your list) and would add "required" to the list (despite the "required requirement" oddity) along with simple "true" (I think this carries a bit of the message that FALSE = not "true" -> false)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Wed 24 Apr 2013 05:14:05 AM UTC, comment #1:

>or by changing the word used to be something positive.

I agree.

>Note that the introduction of nreqs uses an entirely different mechanism than that suggested in patch #3332

The mechanism was wrong and my original idea was confuse. My purpose was the patch #1342, where requirement_vector nreqs replaced obsoleted_by. This was the second step.

The first step was in patch #1339, where requirement_vector reqs replaced the fields require_advance, need_improvement and need_government.

Same for patch #1341 and #1340.

J. M. Gorbach <gorb>
Wed 24 Apr 2013 12:52:01 AM UTC, original submission:

The semantics of the "negated" field in requirements really irritate me. The conditionals in the code generally require reverse polarity and double-negative thinking. Ruleset fragments need careful thought because "TRUE" means that a given requirement must not be met, and "FALSE" means that it must be met.

There are two ways to address this, either by enabling nreqs everywhere (as suggested in patch #3332), or by changing the word used to be something positive. I've prepared patches achieving both, the first against trunk, and the second against trunk + patch #3835 for consideration. I don't believe either is currently perfect for application to trunk, but thought discussion might be improved with code to review. Although the attached patches compile, I have not tested them in gameplay at all.

I personally prefer the use of a positive term, as this better integrates with my work-in-progress towards disjunctive requirements specification, but have not successfully figured out a good word to use for this in the last few weeks of thinking about it: candidates have been "present", "active", "exists", "condition", "test", "asserted", "found", and others that I dismissed before adding them to my TODO documentation (hence the use of the easily search&replaceable term in the patch). Suggestions welcomed gratefully.

Note that the introduction of nreqs uses an entirely different mechanism than that suggested in patch #3332 (part of why I'm not reusing that ticket): specifically reversing the polarity of ruleset defined nreqs when storing them in the reqs vector, rather than introducing nreqs vectors everywhere (with the need for all the attendant plumbing in requirements.c and code review of callers). This doesn't address the double-negation in the code, but it does mask it from ruleset authors to some degree, making rulesets easier to both write and read.

Emmet Hikory <persia>
Project Member

 

(Note: upload size limit is set to 1024 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #17944:  rename-negated-to-present+effects.patch added by persia (48kB - application/octet-stream)
file #17938:  rename-negated-to-present+SVN22836.patch added by persia (48kB - application/octet-stream)
file #17936:  rename-negated-to-present.patch added by persia (50kB - application/octet-stream)
file #17821:  0001-Drop-negated-in-favor-of-nreqs.patch added by persia (13kB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by cazfi (Posted a comment)
  • -unavailable- added by gorb (Posted a comment)
  • -unavailable- added by persia (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 11 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 13 May 2013 09:32:53 PM UTCcazfiStatusReady For Test=>Done
      Open/ClosedOpen=>Closed
    Thu 09 May 2013 09:47:54 PM UTCpersiaAttached File-=>Added rename-negated-to-present+effects.patch, #17944
    Thu 09 May 2013 08:33:26 AM UTCcazfiCategoryrulesets=>general
      StatusNone=>Ready For Test
      Assigned toNone=>cazfi
      Planned Release=>2.6.0
    Thu 09 May 2013 08:23:11 AM UTCpersiaAttached File-=>Added rename-negated-to-present+SVN22836.patch, #17938
    Thu 09 May 2013 06:31:40 AM UTCpersiaAttached File-=>Added rename-negated-to-present.patch, #17936
    Wed 24 Apr 2013 12:52:01 AM UTCpersiaAttached File-=>Added 0001-Drop-negated-in-favor-of-nreqs.patch, #17821
      Attached File-=>Added 0001-Rename-negated-to-something-positive.patch, #17822
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup