bugFreeciv - Bugs: bug #20519, Functionality inside...

 
 
Show feedback again

bug #20519: Functionality inside fc_assert_ret() in unit unloading

Submitted by:  Marko Lindqvist <cazfi>
Submitted on:  Sun Feb 17 10:02:26 2013  
 
Category: generalSeverity: 3 - Normal
Priority: 5 - NormalStatus: Fixed
Assigned to: Marko Lindqvist <cazfi>Open/Closed: Closed
Release: Operating System: None
Planned Release: 2.4.0, 2.5.0Contains string changes: None

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)

Wed Feb 20 09:16:09 2013, SVN revision 22394:

Moved necessary functionality out from fc_assert_ret_val()
Functionality of fc_assert_ret_val() currently happens to be included in
release builds too, so this was not causing real problems, but was
just a style issue.

See gna bug #20519

(Browse SVN revision 22394)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Wed Feb 20 09:16:04 2013, SVN revision 22393:

Moved necessary functionality out from fc_assert_ret_val()
Functionality of fc_assert_ret_val() currently happens to be included in
release builds too, so this was not causing real problems, but was
just a style issue.

See gna bug #20519

(Browse SVN revision 22393)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sun Feb 17 10:58:35 2013, comment #4:

I agree with you.

pepeto <pepeto>
Project Member
Sun Feb 17 10:54:22 2013, comment #3:

> It looks like fc_assert_ret() isn't disabled in NDEBUG builts.


Right. So some macros of the family are affected by NDEBUG and others are not. I can see why it's made that way, but I don't necessarily like the inconsistency. Well, maybe they are fine defined that way, but then we force the patch #3712 CodingStyle policy that no necessary functionality is allowed to be put inside macro calls even in case of those macros for which current implementation allows it. 1) It's easier to remember "Never" than "In case of this and that macro it's ok, but not with those two" 2) things may change in the future.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sun Feb 17 10:38:21 2013, comment #2:

It looks like fc_assert_ret() isn't disabled in NDEBUG builts.

pepeto <pepeto>
Project Member
Sun Feb 17 10:11:32 2013, comment #1:

(Bug #20084 was a previous, very similar instance of this problem.)

Jacob Nevins <jtn>
Project Administrator
Sun Feb 17 10:02:26 2013, original submission:

Functionality to remove cargo from the list of transporter's transported units is inside fc_assert() so it won't happen at all when asserts are disabled.

Fix attached

I'm worried. The fact that there's some such cases of functionality inside fc_assert() indicates that someone has not been aware of its dangers and could have made same mistake in many places.
I'll see if CodingStyle needs updating in this respect.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.

 

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

Attach File(s):
   
   
Comment:
   

Attached Files

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -unavailable- added by pepeto (Posted a comment)
  • -unavailable- added by jtn (Posted a comment)
  • -unavailable- added by cazfi (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.

     

    Error: not logged in

     

     

    Follow 6 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sun Apr 7 10:10:05 2013jtnDependencies-=>bugs #20722 is dependent
    Wed Feb 20 23:17:28 2013jtnSummaryUnloading problem in release builds=>Functionality inside fc_assert_ret() in unit unloading
    Wed Feb 20 09:16:25 2013cazfiStatusReady For Test=>Fixed
      Assigned toNone=>cazfi
      Open/ClosedOpen=>Closed
    Sun Feb 17 10:02:26 2013cazfiAttached File-=>Added ReleaseTransportedList.patch, #17234
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup